Re: IncomingFileTransfer recieveFile(..) throws "error in execution"

I am having the same issue after upgrading to 4.1.0-rc1 (from 3.5).

After two days of debugging (I should have come earlier to this thread :-() ) , I think I pinpointed the problem to this code in the StreamNegotiator:

Stanza initiateIncomingStream(XMPPConnection connection, StreamInitiation initiation) throws NoResponseException, XMPPErrorException, NotConnectedException {

StreamInitiation response = createInitiationAccept(initiation,

getNamespaces());

// establish collector to await response

PacketCollector collector = connection

.createPacketCollectorAndSend(getInitiationPacketFilter(initiation.getFrom(), initiation.getSessionID()), response);

Stanza streamMethodInitiation = collector.nextResultOrThrow();

return streamMethodInitiation;

}

Smack is generating a IQ result message, which is in fact a result/response of a set IQ, but then start to wait for a response on this result. There is of course no real response, but instead a IQ set is sent (or even two) with the correct From address and SessionID. This IQ set is not picked up by the collector, while – and this is the part I do not understand – in the AbstractXMPPConnection invokePacketCollectorsAndNotifyRecvListeners IQ set and get stanz are handled different, and explicitly is said:

// The following returns makes it impossible for packet listeners and collectors to

// filter for IQ request stanzas, i.e. IQs of type ‘set’ or ‘get’. This is the

// desired behavior.

return;

So either there should be no collector but another type of stanza listener, or the code in the invokePacketCollectorsAndNotifyRecvListeners should be elaborated.

To summarize:

client 1 (anno…) is sending:

http://jabber.org/protocol/bytestreamshttp://jabber.org/protocol/ibb

client 2 (test1…) is answering:

http://jabber.org/protocol/bytestreamshttp://jabber.org/protocol/ibb

and waiting for an answer from “anno…” and sid=‘jsi_8237492034875763688’.

Client 1 is sending/responding with even two answers:

but client 2 neither of them accepts them as a proper response, while they are IQ set stanza and not presented to any collector.

Thanks Anno for you detailed analysis. As far as I can tell it is correct.

But I should maybe start with some background. XMPP requires entities to send a response to an IQ request (there are some exceptions from this rule, e.g. when you want to prevent presence leaks). Before Smack 4.1, this rule was roughly implemented: Every time Smack was not able to parse an IQ request, it would send the required error response, assuming that if it was able to parse the request, then someone would actually take care of it.

This approach had some drawbacks:

  • XMPP logic was coupled with parsing. The result was that the parseIQ method was defined with a second parameter besides the obvious XmlPullParser: A XMPPConnection. This connection was used to send the error IQ responses in case Smack was not able to parse the stanza.

  • It was not guaranteed that an error result was sent, in case a provider existed but no logic within Smack to handle the IQ request.

  • There could be multiple instances handling the requests, resulting in multiple responses

Some time ago, I introduced the concept of IQ request handlers in Smack 4.1. If you want to receive IQ requests, you have to register such a handler. Only one handler can be active for a given IQ request “class”. And if no handler is active, Smack would send an error response (if replyToUnknownIq is set).

I did some tests, took a careful look at the code, but unfortunately I missed that the logic in StreamNegotiator broke with the change. The unit tests for file transfer look pretty exhaustive on a first quick look, but it appears they also didn’t catched that. I really wish we could resurrect the integration tests in Smack, they probably would had catch that.

I’m currently working on an approach to fix StreamNegoiator. It appears that it will be a quick and dirty hack, because how file transfer is implemented in Smack. And since I’m not happy with the design and implementation of the file transfer API in Smack, I hope that in the long run we’ll get proper Jingle file transfers with a new and thoughtful design and implemented API.

FYI: I’ve branched this thread from IncomingFileTransfer recieveFile(…) throws “error in execution”

I’ve created a branch which HEAD reflects my current attempt to solve this: https://github.com/Flowdalic/Smack/tree/incomingFtFix

Although this is completely untested and WIP, feedback is always welcomed. You are even encouraged to try it out and see if it fixes the issue.

Anno, same as you, I have an app built on 3.5 that depends on IncommingFileTransfer. 4.* has been difficult. I spent days debugging this problem and your problem statement matches the IQ set/get traces sequence I posted: IncomingFileTransfer recieveFile(…) throws “error in execution”

Flow, I’ll test https://github.com/Flowdalic/Smack/tree/incomingFtFix. My application test will be out-of-band at a scale of N file transfers over a time period, all directed at a single class instance who implements the public interface named, FileTransferListener.

Great, thanks.

But don’t expect to much yet, as I was myself not able to test it. I may have time to do so in the next few days.

Good news. I think I fixed the issue, at least my personal integration tests, which can be found at smack-examples/FiletransferTest.java at master · Flowdalic/smack-examples · GitHub are able to perform file transfers again.

The rc3-SNAPSHOT with the fix is uploaded as I write this. Please test and report back. Thank you.

Did some testing and found out that the In-Band Bytestream did not work.

While I was not sure the cause was in my code, I set myself to fixing this bug, which i could pinpoint to a missing IQRequestHandler for Data IQ packets.

I uploaded a patch to my fork of smack annovanvliet/Smack at incomingFtFix · GitHub

Thanks Anno,

I’ve made minor modifications to your patch and added a second commit that removes the stanza listener related code from DataListener since it’s now a plain IqRequestHandler.

I’m currently uploading a rc3-SNAPSHOT that includes all the fixes. As always, please test and report back.

I’ve also fixed (I think) the outstanding stream management issue. Therefore I hope that all blockers for the 4.1 release are now solved and that I can release soon a last rc3 before 4.1

@Anno van Vliet @mike any news? I’m basically waiting for your feedback to proceed further. How does 4.1.0-rc3-SNAPSHOT work for you?

After the last small fix for inbound it works for me.

I tested using M-link (16.2v12) and TCS (Tactical Chat 1.0.4), using my own clients and with an old version of my client, which uses an old version of smack 3.4.1. I did not do any performance or load testing.

Today I will start testing your changes and report back my findings by end of day.

Using Gradle, I quickly built https://github.com/Flowdalic/Smack/tree/incomingFtFix to get the needed jars for this test. Here are my results:

  • My connection code would not compile because setServiceName now takes a DomainBareJid. So I found singleton JidCreate.domainBareFrom(…), which produces a DomainBareJid. IMHO, this new class is easy to use but will cause upgrade pains for some.
  • When I tried to connect to my XMPP Server using Builder idiom, I got exception below. Since I don’t have a valid connection, can’t test File Transfer Listener that will exercise incoming FtFix. Sorry to get off track here…

Exception in thread “Smack Packet Reader (0)” java.lang.AssertionError

at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.parsePackets(XMPPTCPC onnection.java:983)

at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader.access$200(XMPPTCPCon nection.java:928)

at org.jivesoftware.smack.tcp.XMPPTCPConnection$PacketReader$1.run(XMPPTCPConnecti on.java:943)

at java.lang.Thread.run(Thread.java:745)

Using Gradle, I quickly built https://github.com/Flowdalic/Smack/tree/incomingFtFix to get the needed jars for this test.

There is no need to build it yourself. Just point your build system to use Smack 4.1.0-rc3-SNAPSHOT and make sure that it uses the latest available snapshot.

My connection code would not compile because setServiceName now takes a DomainBareJid.

That sounds like you build the ‘master’ branch instead of a 4.1 branch.

Please tell me where the Smack 4.1.0-rc3-SNAPSHOT jars are.

Using Maven The Central Repository Search Engine

g:“org.igniterealtime.smack” AND v:“4.1.0-rc3”

returns nothing

I found them on sonatype.org.

In my maven project I added the following to retrieve them :

smack-sonatypehttps://oss.sonatype.org/content/repositories/snapshots/truetrue</enab led>default

Using 4.1.0-rc3-SNAPSHOT, my file transfer testing all PASSED . I still need to test some more IIB versus out-of-band scenarios tomorrow.

Based on recent 4.0.4 experiences, I wonder if pubsub has the same kind of problem that you’ve fixed here on File Transfer?

When do we anticipate this being released and what version will be targeted?

I wonder if pubsub has the same kind of problem that you’ve fixed here on File Transfer?
“same kind of problem”? I don’t think that this is related to Receiving pubsub messages with JSON Payload and I’ve got reports that pubsub works on 4.1.0-rc3.