Skip to content

Commit d66c21a

Browse files
committed
Use try_emplace in SetThread instead of threads.find
This commit easiest to review ignoring whitespace (git diff -w). This is a minor change in behavior, but the only change is shortening the duration that threads.mutex is locked while inserting a new entry in the threads.ref map. The lock is now only help while the entry is created and is released while the ProxyClient<Thread> object is initialized. This change doesn't really fix any problems but it simplifies the next commit which deals with race conditions and deadlocks in this code, so it has been split out.
1 parent 2ad8337 commit d66c21a

File tree

1 file changed

+23
-22
lines changed

1 file changed

+23
-22
lines changed

src/mp/proxy.cpp

Lines changed: 23 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -306,29 +306,30 @@ bool EventLoop::done() const
306306

307307
std::tuple<ConnThread, bool> SetThread(GuardedRef<ConnThreads> threads, Connection* connection, const std::function<Thread::Client()>& make_thread)
308308
{
309-
const Lock lock(threads.mutex);
310-
auto thread = threads.ref.find(connection);
311-
if (thread != threads.ref.end()) return {thread, false};
312-
thread = threads.ref.emplace(
313-
std::piecewise_construct, std::forward_as_tuple(connection),
314-
std::forward_as_tuple()
315-
).first;
316-
thread->second.emplace(make_thread(), connection, /* destroy_connection= */ false);
317-
thread->second->setDisconnectCallback([threads, thread] {
318-
// Note: it is safe to use the `thread` iterator in this cleanup
319-
// function, because the iterator would only be invalid if the map entry
320-
// was removed, and if the map entry is removed the ProxyClient<Thread>
321-
// destructor unregisters the cleanup.
322-
323-
// Connection is being destroyed before thread client is, so reset
324-
// thread client m_disconnect_cb member so thread client destructor does not
325-
// try to unregister this callback after connection is destroyed.
326-
// Remove connection pointer about to be destroyed from the map
309+
ConnThread thread;
310+
bool inserted;
311+
{
327312
const Lock lock(threads.mutex);
328-
thread->second->m_disconnect_cb.reset();
329-
threads.ref.erase(thread);
330-
});
331-
return {thread, true};
313+
std::tie(thread, inserted) = threads.ref.try_emplace(connection);
314+
}
315+
if (inserted) {
316+
thread->second.emplace(make_thread(), connection, /* destroy_connection= */ false);
317+
thread->second->setDisconnectCallback([threads, thread] {
318+
// Note: it is safe to use the `thread` iterator in this cleanup
319+
// function, because the iterator would only be invalid if the map entry
320+
// was removed, and if the map entry is removed the ProxyClient<Thread>
321+
// destructor unregisters the cleanup.
322+
323+
// Connection is being destroyed before thread client is, so reset
324+
// thread client m_disconnect_cb member so thread client destructor does not
325+
// try to unregister this callback after connection is destroyed.
326+
// Remove connection pointer about to be destroyed from the map
327+
const Lock lock(threads.mutex);
328+
thread->second->m_disconnect_cb.reset();
329+
threads.ref.erase(thread);
330+
});
331+
}
332+
return {thread, inserted};
332333
}
333334

334335
ProxyClient<Thread>::~ProxyClient()

0 commit comments

Comments
 (0)