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)
Smack currently does unnecessary escaping of XML text, where it
escapes e.g. '"' to '"'. This bloats the stanza size, especially
if JSON payloads are involved.
Fixes SMACK-892 (although there are probably still places where
XmlStringBuilder.escape() is used when XmlStringBuild.text() could
have been used).
This means that users get now exceptions with helpful error messages
instead of the dreaded ClassCastException, like
java.lang.ClassCastException: org.jivesoftware.smack.packet.StandardExtensionElement cannot be cast to org.jivesoftware.smackx.mam.element.MamElements$MamResultExtension
at
when StanzaView.getExtension(Class) is used to retrieve the extension.
When sending a stream-open-like element, it depends on the actual used
transport which element is send. For example, RFC6120-style TCP uses
<stream>, whereas the Websocket binding for XMPP uses <open/>.
Most of the times when we construct a stream-open-like element, we
want the jabber:client namespace. Hence add a constructor that does
select the namespace implicitly.
Before the existence of AbstractStreamOpen, StreamOpen sufficed our need
during sending an open stream element. Since the intention behind
introducing AbstractStreamOpen is to allow underlying transports provide
transport specific opening streams, these changes will further support
the cause.
This commit will allow us to send transport specific open element
which should be inherited from AbstractStreamOpen.
This PR aims to provide parseXrdLinkReferencesFor() method the ability
to parse forward to the first START_ELEMENT tag.The HttpLookupMethodTest
tests the HttpLookupMethod class by parsing String. This makes use of
PacketParserUtils.getParserFor(String), which already does forward
winding to reach START_ELEMENT. However when fetching endpoints from a
remote host meta data, PacketParserUtils.getParserFor(InputStream) is
used which doesn't do winding in any form. And thus, even though
HttpLookupMethodTest tests pass, this implementation would crash while
parsing remote host-meta.
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 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.
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.
This moves the logic in AbstractXMPPConnection.getSmackTlsContext()
into the ConnectionConfiguration constructor.
Also introduce SslContextFactory and use it in
ConnectionConfiguration.
The field XMPPInputOutputStream.flushMethod was not initialized, which
could cause an NPE in the "switch (flushMethod)" found in
Java7ZlibInputOutputStream.getOutputStream().
Apply builder pattern to form fields and replace getVariable() with
getFieldName(). Refer to the field name as "field name" instead of
"variable" everyone, just as XEP-0004 does.
Improve the high-level form API: introduce FilledForm and FillableForm
which perform stronger validation and consistency checks.
Also add FormFieldRegistry to enable processing of 'submit' forms
where the form field types are omitted.
Smack also now does omit the form field type declaration on 'submit'
type forms, as it is allowed by XEP-0004.
The initState() method is also called in disconnect(). And if we reset
the closingStreamReceived sync point at disconnect, it will break the
WaitForClosingStreamElementTest integration test.
Return false as soon as the hashed value does not match. This is
sound, since every class that implements equals(Object) should also
implement hashCode().
This method was removed with 07da9ffb4 ("Do not have
Stanza.getExtension(String, String) return a generic type"). In order
to aide migration to the newer API, this commit re-adds the method and
marks it as deprecated.
This also removes the powermock dependency. Although powermock is a
fine library, it currently prevents dropping Junit4. And since we only
use the Whitebox API of powermock, this simply replaced powermock's
Whitebox with our own.
Ensure that he returned extension element is actually of the correct
type. It should not be common, but in theory, Smack's provider
architecture would allow for different types to be returned than the
ones one may expect.
The code in the integration-test/ directories should either be
migrated to sinttest (or unit tests), or get deleted. This is a first
small step towards this goal.
Returning a generic would allow for
List<ExtensionElement> list = stanza.getExtension("foo", "bar");
to compile (Note the we are calling getExtension(), not
getExtension*s*()).
Users are encouraged to use the type safe getExtension(Class<? extends
ExtensionElement) variant instead.
Fixes SMACK-825.
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.
By not directly depending on Bouncycastle (BC), we avoid conflicts between
different bouncycastle versions. It is also part of the developers job
to take care that all required security primitives are available. If
they are provide by BC or some other security provider should not be
up to Smack to decide.
We now only add BC as test dependency to satisfy this requirement when
the unit tests are executed.
Instead of marking the handle as not running by setting the handler's
value in the map to false, we now remove simply the key if there is no
handler running. This also means we no longer need to use a weak hash
map for this.
Also reduce the size of the synchronized blocks, mainly by scheduling
the handler outside of the synchronized(threadActiveMap) block.
Make some code better readable and add some more comments. Also do
start a new handler thread if the task threw.
If we do not peek at the scheduled actions in the reactors
synchronized block, then there is a kind of lost-update problem. While
Ractor.schedule() will call wakeup() on the selector, a thread could
have already determined the value of selectWait, while being blocked
at the start of the synchronized reactor section. Once it is able to
enter the section, it will use an outdated selectWait value.
This leads to scheduled actions not being executed on time.
Thanks to Eng ChongMeng for reporting this and suggesting the fix.
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.
Florian Schmaus
adiaholic
Paul Schaub
Guus der Kinderen