InBandBytestreamManager followed an unusual pattern: Within the
connectionTermianted() callback, it would remove itself from the
'managers' map. This allowed for multiple instances of an
InBandBytestreamManager to exist for the same connection, causing all
kinds of issues.
This fixes the issue by changing InBandBytestreamManager to use the
Smack-idiomatic pattern used by managers.
We also do no longer reset the listeners if the connection is
termianted, as listeners (and handlers) typically persist until they
are explicitly removed by the user.
As positive side-effect, the number of indeterministic unit-tests,
caused by using Thread.sleep(), is reduced. The executor service in
InitiationListener was also removed, because the IQ handler is already
called asynchronously to the connections main loop.
Thanks to Anno van Vliet for reporting this.
To ensure the thread starting the reader/writer threads sees them
running and eventually waits until the 'running' boolean is reset to
'false' upon connection termination.
Since the current variant of notifyConnectionError() does not execute
most of its work in a new thread, especially since instantShutdown()
is called in the invoking thread, we have to mark the connections
reader or writer threads as no longer running prior them invoking
notifyConnectionError(). Otherwise they will end up waiting for
themselves to terminate.
To make it clear that this will either return if the condition is
true *or* if a connection exception happened.
Also introduce waitFor(), which is deliberately not named
waitForCondition() because it carries a different semantic.
SMACK-888 has a mention of unreachable code and is now solved
under commit 0f7b7df.
This integration test tests those class entities which were to
be set by previously stated unreachable code.
Synchronize notifyConnectionError() so that only one exception is
handled and remove the ASYNC_BUT_ORDERED usage here. The
ASYNC_BUT_ORDERED was added with 7d2c3ac9f ("Do not call synchronized
methods in reader/writer thread"), but is no longer necessary, since
the Semaphores where replaced with conditions in the previous commit.
This continues the design started with e98d42790 ("SmackReactor/NIO,
Java8/Android19, Pretty print XML, FSM connections"), where the
exceptions that caused an operation to fail, are not recorded within
SynchronizationPoint but within the connection instance itself.
Since d65f2c932 ("Bump Error Prone version to 2.3.4 and fix new bug
patterns") the channel selected callback is no longer a final field of
the connection instance, hence it may be come null even if the
connection instance is still strongly referenced. Also the
ConnectionAttemptState class uses simply a lambda as callback, which
is also not strongly referenced otherwise.
The "channel selected" callback was wrapped in weak reference, so that
connection instances could get gc'ed if they are still connected but
the user lost all references to them. In this case, the weak reference
to the connection instance would become 'null' and
selectionKey.cancel() would be called.
This change means that a socket and its selection key of a "leaked"
connected connection instance continues to be part of the reactor. But
this may not be that bad: first, users are expected to manager their
connection instances, and disconnect them before they are
discarded. And secondly, at some point the connection likely will get
disconnected, and in this case, the socket and its selection key will
be removed from the reactor.
