wsd: protect against thread-affinity violation in StreamSocket dtor

[Ashod]

As the comment explains, there are scenarios
when the socket is closed and removed while
it's held by a temporary shared_ptr acquired
through a weak_ptr in another thread. In such
a scenario, the temp shared_ptr will call the
destructor in its incorrect thread, causing
the thread-affinity assertion to fire inside
of ensureDisconnected().

This isn't helpful for the main reason that
the ensureDisconnected() isn't necessary and
will not do anything but check that there is
nothing to be done! And since avoiding the
destruction in a different thread than the
one we have affinity to is rather complex
and tricky, we avoid the issue by skipping
the ensureDisconnected() call when it's not
needed.

Another fix would've been to reset the thread
affinity, but that seems to be too blunt a
tool and robs us of the opportunity of catching
affinity violations when ensureDisconnected()
isn't invoked on destruction while we are
destroyed from the wrong thread.

Change-Id: I8e41ee56a4203aaba3d21472c9d1a446cb76f9a2
Signed-off-by: Ashod Nakashian ashod.nakashian@collabora.co.uk

In general, the fix is to ensure that constructors and destructors of base classes do not cause virtual dispatch, directly or indirectly. Here, the destructor of StreamSocket calls ensureDisconnected(), which in turn calls the virtual shutdownConnection(). We should refactor so that ensureDisconnected() performs only non-virtual logic and leaves the actual shutdown call to a context where virtual dispatch is appropriate, or to an explicit base-class call.

The simplest change without altering external behavior is:

• Keep StreamSocket::~StreamSocket() as-is in terms of visible effects (it still performs disconnect logic and then calls StreamSocket::shutdownConnection() explicitly).
• Modify ensureDisconnected() so it no longer calls asyncShutdown() and shutdownConnection(). That way, when it is invoked from the destructor we do not indirectly invoke any virtual function. We still maintain its primary contract: emitting onDisconnect() once and logging if the socket remains open.
• Shutdown during destruction continues to be handled exclusively by the explicit call to StreamSocket::shutdownConnection() in the destructor (StreamSocket::shutdownConnection();), which is a non-virtual qualified call and therefore safe in a destructor.
• In non-destruction contexts, callers that require both disconnect notification and shutdown can perform ensureDisconnected(); asyncShutdown(); shutdownConnection(); themselves if needed. Since we cannot see other callers in this snippet, we avoid modifying them; we only narrow ensureDisconnected()’s responsibilities, which is less likely to break logic because the destructor already performs shutdown unconditionally.

Concretely, in net/Socket.hpp:

• In the body of ensureDisconnected(), remove the block that conditionally calls asyncShutdown(); and shutdownConnection(); when !isShutdown(). Leave the rest of the method unchanged.

This eliminates the virtual call from the destructor while preserving existing destruction semantics: onDisconnect() remains called once, and shutdown is still performed via the explicit base call in the destructor.

[mmeeks]

Ooh - I worry about this case I guess; are we sure that all of this cleanup happens properly in the other threads ? eg. ChildSession::disconnect called from ~ChildSession - from ~ProtocolHandlerInterface looks problematic as an example. I assume we are still warning of thread affinity in that chain though (?) =)

Anyhow - it seems quite plausible, and simple enough. Not sure we want a novel in the comment there though - should be visible from the commit message.

[mmeeks]

Fine - but can we comment less verbosely there and leave it to the commit msg :-) or better a cool# ticket reference ?

[mmeeks]

Ooh - I worry about this case I guess; are we sure that all of this cleanup happens properly in the other threads ? eg. ChildSession::disconnect called from ~ChildSession - from ~ProtocolHandlerInterface looks problematic as an example. I assume we are still warning of thread affinity in that chain though (?) =)

Anyhow - it seems quite plausible, and simple enough. Not sure we want a novel in the comment there though - should be visible from the commit message.