-
Type:
Bug
-
Resolution: Done
-
Priority:
Trivial - P5
-
None
-
Affects Version/s: 2.5.5
-
Component/s: Networking
-
None
-
Platforms 2018-05-07
-
0
-
None
-
0
-
None
-
None
-
None
-
None
-
None
-
None
Affected files:
- util/net/listen.h
- util/net/listen.cpp
The sockets created by Listener::setupSockets() and stored in Listener::_socks are also passed to ListeningSockets::add(). This creates a point of dual ownership, where ListeningSockets::closeAll() is responsible for closing sockets that Listener::initAndListen() loops over until inShutdown().
More specifically the race comes from:
- Listener::setupSockets() called
- some number of sockets are created
- sockets are added to Listener::_socks
- sockets are added to ListeningSockets::add()
- Listener::initAndListen() called
- The Listener begins looping over the sockets it created. It does this until inShutdown()
- ListeningSockets::closeAll() called
- the mutex doesn't do anything because Listener has the same set of sockets in it's own private vector
- Sockets are all closed immediately
- Listener::initAndListen() asserts with an invalid socket from either select() or WSAEventSelect()
- one of the sockets Listener is iterating over has been closed, and is no longer valid
I'm not entirely clear on whether the server manages this shutdown cleanly (by putting the Listener into inShutdown() before calling closeAll()), or if calling dbexit() leads to an _exit() fast enough to skip by the race. But either way it's awkward in the extreme to make another, ostensibly thread safe, class responsible for resources that Listener owns and continuously works on.
Ideally Listener would either be responsible for it's own sockets, or ListeningSockets would have some kind of api for checking sockets out for a duration to allow for actual thread safe processing.