Do net put an ack to the queue if it has already been shutdown. Some
servers, like ejabberd, like to request an ack even after we have send
a stream close (and hance the queue was shutdown). If we would not
check here, then the ack would dangle around in the queue, and be send
on the next re-connection attempt even before the stream open.
See the following trace of the MUC bookmarks integration test. The
fact that it is a MUC test does not matter, but this test does
disconnect the connection and reconnect it. Not how the server,
ejabberd in this case, requests an SM ack by sending an <r/> even
though we already send the </stream:stream>:
22:22:05 SENT (4):
<iq id='MD4UC-61' type='set'>
<query xmlns='jabber:iq:private'>
<storage xmlns='storage:bookmarks'>
<conference name='Smack Inttest: 7in7j' autojoin='true' jid='y9jcn5@conference.salem.geekplace.eu'>
<nick>
Nick-P2VXD7
</nick>
</conference>
</storage>
</query>
</iq>
22:22:05 RECV (4):
<r xmlns='urn:xmpp:sm:3'/>
22:22:05 SENT (4):
<a xmlns='urn:xmpp:sm:3' h='29'/>
22:22:05 RECV (4):
<message to='sinttest-7in7j-4@salem.geekplace.eu' from='sinttest-7in7j-4@salem.geekplace.eu' type='headline'>
<event xmlns='http://jabber.org/protocol/pubsub#event'>
<items node='storage:bookmarks'>
<item id='current'>
<storage xmlns='storage:bookmarks'>
<conference name='Smack Inttest: 7in7j' autojoin='true' jid='y9jcn5@conference.salem.geekplace.eu'>
<nick>
Nick-P2VXD7
</nick>
</conference>
</storage>
</item>
</items>
</event>
<addresses xmlns='http://jabber.org/protocol/address'>
<address jid='sinttest-7in7j-4@salem.geekplace.eu/1415073683806426185213090' type='replyto'/>
</addresses>
</message>
22:22:05 RECV (4):
<iq xml:lang='en-US' to='sinttest-7in7j-4@salem.geekplace.eu/1415073683806426185213090' from='sinttest-7in7j-4@salem.geekplace.eu' type='result' id='MD4UC-61'/>
22:22:05 SENT (4):
<presence id='6MS6J-20' type='unavailable'/>
<a xmlns='urn:xmpp:sm:3' h='31'/>
<!-- We have closed the stream -->
</stream:stream>
<!-- But the server still requests an SM ack -->
22:22:05 RECV (4):
<r xmlns='urn:xmpp:sm:3'/>
22:22:05 RECV (4):
</stream:stream>
22:22:05 XMPPConnection closed (XMPPTCPConnection[sinttest-7in7j-4@salem.geekplace.eu/1415073683806426185213090] (4))
22:22:05 SENT (4):
<a xmlns='urn:xmpp:sm:3' h='31'/>
22:22:05 SENT (4):
<stream:stream xmlns='jabber:client' to='salem.geekplace.eu' xmlns:stream='http://etherx.jabber.org/streams' version='1.0' from='sinttest-7in7j-4@salem.geekplace.eu' xml:lang='en-US'>
22:22:05 RECV (4): ?xml version='1.0'?>
<stream:stream id='3379123514446782311' ver
22:22:05 RECV (4): sion='1.0' xml:lang='en' xmlns:stream='http://etherx.jabber.org/streams' xmlns='jabber:client'>
<stream:error>
<invalid-xml xmlns='urn:ietf:params:xml:ns:xmpp-streams'/>
</stream:error>
</stream:stream>
22:22:05 XMPPConnection closed due to an exception (XMPPTCPConnection[sinttest-7in7j-4@salem.geekplace.eu/1415073683806426185213090] (4))
org.jivesoftware.smack.XMPPException$StreamErrorException: invalid-xml You can read more about the meaning of this stream error at http://xmpp.org/rfcs/rfc6120.html#streams-error-conditions
<stream:error><invalid-xml xmlns='urn:ietf:params:xml:ns:xmpp-streams'/></stream:error>
at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.parsePackets(XMPPTCPConnection.java:981)
at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.access$700(XMPPTCPConnection.java:913)
at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader$1.run(XMPPTCPConnection.java:936)
at java.base/java.lang.Thread.run(Thread.java:834)
We previously only set the SM session ID to zero, but that is not
enough. On a clean shutdown, i.e. where we send a </stream> close tag,
we also have to nullify the unacknowledgedStanzas queue.
We previously only set 'connected' after connectInternal()
returned. This could lead to notifyConnectionError() ignoring stream
error exceptions, e.g. when establishing TLS which happens also in
connectInternal(), because 'connected' was still 'false'.
2020-08-06 13:08:06.265 19830-20423/org.atalk.android D/SMACK: SENT (0):
<stream:stream xmlns='jabber:client' to='atalk.sytes.net' xmlns:stream='http://etherx.jabber.org/streams' version='1.0' xml:lang='en'>
2020-08-06 13:08:06.333 19830-20424/org.atalk.android D/SMACK: RECV (0): ?xml version='1.0'?>
<stream:stream id='16420577292739412012' version='1.0' xml:lang='en' xmlns:stream='http://etherx.jabber.org/streams' from='atalk.sytes.net' xmlns='jabber:client'>
<stream:error>
<policy-violation xmlns='urn:ietf:params:xml:ns:xmpp-streams'/>
<text xml:lang='en' xmlns='urn:ietf:params:xml:ns:xmpp-streams'>
Too many (20) failed authentications from this IP address (::ffff:42.60.7.13). The address will be unblocked at 05:15:34 06.08.2020 UTC
</text>
</stream:error>
</stream:stream>
2020-08-06 13:08:06.346 19830-20424/org.atalk.android I/aTalk: [241896] org.jivesoftware.smack.AbstractXMPPConnection.notifyConnectionError() Connection was already disconnected when attempting to handle org.jivesoftware.smack.XMPPException$StreamErrorException: policy-violation You can read more about the meaning of this stream error at http://xmpp.org/rfcs/rfc6120.html#streams-error-conditions
<stream:error><policy-violation xmlns='urn:ietf:params:xml:ns:xmpp-streams'/><text xml:lang='en'>Too many (20) failed authentications from this IP address (::ffff:42.60.7.13). The address will be unblocked at 05:15:34 06.08.2020 UTC</text></stream:error>
org.jivesoftware.smack.XMPPException$StreamErrorException: policy-violation You can read more about the meaning of this stream error at http://xmpp.org/rfcs/rfc6120.html#streams-error-conditions
<stream:error><policy-violation xmlns='urn:ietf:params:xml:ns:xmpp-streams'/><text xml:lang='en'>Too many (20) failed authentications from this IP address (::ffff:42.60.7.13). The address will be unblocked at 05:15:34 06.08.2020 UTC</text></stream:error>
at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.parsePackets(XMPPTCPConnection.java:966)
at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.access$700(XMPPTCPConnection.java:898)
at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader$1.run(XMPPTCPConnection.java:921)
at java.lang.Thread.run(Thread.java:919)
Which eventually leads to a NoResponseException
org.jivesoftware.smack.SmackException$NoResponseException: No response
received within reply timeout. Timeout was 30000ms (~30s). While
waiting for establishing TLS
[XMPPTCPConnection[not-authenticated] (4)]
We now set 'connected' to 'true' as soon as the transport (e.g. TCP,
BOSH, …) is connected. While this is in other ways also sensible, it
also allows notifyConnectionError() to handle exceptions in the early
connection stage.
Thanks to Eng Chong Meng 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.
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.
This moves the logic in AbstractXMPPConnection.getSmackTlsContext()
into the ConnectionConfiguration constructor.
Also introduce SslContextFactory and use it in
ConnectionConfiguration.
This is a complete redesign of what was previously
XmppNioTcpConnection. The new architecture allows to extend an XMPP
client to server (c2s) connection with new transport bindings and
other extensions.
Besides the way the transport handles the stream after SASL
<success/>, the SASL logic is independend from the underlying
transport (BOSH, TCP, …). Hence move it up into
AbstractXMPPConnection.
This also has the benefit that we can make some more methods private
or package-private.
Also introduce XmlStringBuilder.optTextChild(), which causes some
associated changes.
This is based on the patch provided by Georg Lukas, modified to take
XMPP domain names that can not be represented as DNS names into
account. See c42d2eea24 (r269250137)
Fixes SMACK-870.
Introducing Smack's own XmlPullParser interface which tries to stay as
compatible as possible to XPP3. The interface is used to either wrap
StAX's XMLStreamReader if Smack is used on Java SE, and XPP3's
XmlPullParser if Smack is used on on Android.
Fixes SMACK-591.
Also introduce JUnit 5 and non-strict javadoc projects.
and also call notifyConnectionError() on exception thrown by
openStream().
In hindsight I wonder why openStream() was ever called in the writer
thread, as it only caused unnecessary synchronization overhead, as can
be seen by the initialOpenStreamSend synchronization point.
In case an exception happens in connect()/login() the
'disconnectedButResumable' boolean may (still) be set. Which causes
only one of the reader and writer threads to exit, typically the
reader thread, because shutdown() will bail out very early. This
leaves a dangling (writer) thread causing memory leaks and deadlocks
on a subsequent connect()/login().
as it is not neccessary and causes stalls and deadlocks with a
concurrent connect()/login() (which could be resolved interrupting the
connect()/login() calling thread):
"Smack Reader (0)" daemon prio=5 tid=21 Blocked
| group="main" sCount=1 dsCount=0 obj=0x12c84670 self=0x7e072ca200
| sysTid=14965 nice=0 cgrp=default sched=0/0 handle=0x7e06155450
| state=S schedstat=( 63430626 3034269 21 ) utm=5 stm=0 core=2 HZ=100
| stack=0x7e06053000-0x7e06055000 stackSize=1037KB
| held mutexes=
at org.jivesoftware.smack.tcp.XMPPTCPConnection.notifyConnectionError(XMPPTCPConnection.java:-1)
- waiting to lock <0x0ec6e6bb> (a org.jivesoftware.smack.tcp.XMPPTCPConnection) held by thread 22
at org.jivesoftware.smack.tcp.XMPPTCPConnection.access$3900(XMPPTCPConnection.java:154)
at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.parsePackets(XMPPTCPConnection.java:1330)
at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.access$900(XMPPTCPConnection.java:1064)
at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader$1.run(XMPPTCPConnection.java:1081)
at java.lang.Thread.run(Thread.java:761)
"connect" prio=5 tid=22 Waiting
| group="main" sCount=1 dsCount=0 obj=0x12c8e820 self=0x7e19bcd600
| sysTid=15047 nice=0 cgrp=default sched=0/0 handle=0x7e05ee4450
| state=S schedstat=( 14067083 7475835 14 ) utm=1 stm=0 core=0 HZ=100
| stack=0x7e05de2000-0x7e05de4000 stackSize=1037KB
| held mutexes=
at java.lang.Object.wait!(Native method)
- waiting on <0x0c058b14> (a java.lang.Object)
at java.lang.Thread.parkFor$(Thread.java:2127)
- locked <0x0c058b14> (a java.lang.Object)
at sun.misc.Unsafe.park(Unsafe.java:325)
at java.util.concurrent.locks.LockSupport.park(LockSupport.java:161)
at java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:840)
at java.util.concurrent.locks.AbstractQueuedSynchronizer.doAcquireSharedInterruptibly(AbstractQueuedSynchronizer.java:994)
at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireSharedInterruptibly(AbstractQueuedSynchronizer.java:1303)
at java.util.concurrent.Semaphore.acquire(Semaphore.java:446)
at org.jivesoftware.smack.tcp.XMPPTCPConnection.initConnection(XMPPTCPConnection.java:685)
at org.jivesoftware.smack.tcp.XMPPTCPConnection.connectInternal(XMPPTCPConnection.java:942)
at org.jivesoftware.smack.AbstractXMPPConnection.connect(AbstractXMPPConnection.java:417)
- locked <0x0ec6e6bb> (a org.jivesoftware.smack.tcp.XMPPTCPConnection)
at org.yaxim.androidclient.service.SmackableImp.connectAndLogin(SmackableImp.java:717)
- locked <0x0ec6e6bb> (a org.jivesoftware.smack.tcp.XMPPTCPConnection)
at org.yaxim.androidclient.service.SmackableImp.doConnect(SmackableImp.java:304)
at org.yaxim.androidclient.service.SmackableImp$5.run(SmackableImp.java:391)
If a stream resume fails, smack will re-send all queued stanzas after a
reconnect. However, it does not make sense to re-send them:
* IQs / IQ responses have probably timed out
* MUC messages and PMs will be rejected as you haven't rejoined yet
* regular messages should be amended with a <delay> element
This patch adds a StanzaDroppedListener interface to the
XMPPTCPConnection. If at least one StanzaDroppedListener is provided,
all queued messages will be drained into the StanzaDroppedListener(s).
Otherwise, the original behavior of attempting to transmit them will be
followed.
Discussion: https://discourse.igniterealtime.org/t/xep-0198-resume-failure-reconnect-resending-of-muc-messages/83510/3
Signed-off-by: Georg Lukas <georg@op-co.de>
Florian Schmaus
Georg Lukas