So I just spent about 2 hours tracking down a really simple error in WF, but with a teeny bit of error handling I would have saved all that time.
In my custom client I was trying to revoke membership to a members-only MUC from one owner by another owner. Apparently, this is not allowed by the protocol. The WF code even has a NotAllowedException that is thrown for that case…however, here’'s the catch block in MUCRoomImpl:
catch (NotAllowedException e) {
// We should never receive this exception....in theory
}
The overconfidence in theory notwithstanding, a little e.printStacktrace() right there would have just made my day. Without that, all I had was a NPE at a higher level in the code that I couldn’'t make much sense of.
Also, in section 10.4 of JEP 0045 (Multi-User Chat), the following requirement is made:
If an implementation does not allow one owner to revoke another user’'s ownership privileges, the implementation MUST return a error to the owner who made the request.+
I’'m assuming that WF does not allow this. However, I found that the server simply does not return a result IQ packet when a client attempts to do this (which generally would cause clients to hang until the response times out). So the problem is really two-fold: a lack of low level error handling to at least provide a stack trace, as well as not living up to the protocol laid out in the JEP.
I’'m not gonna object to your requests of better error handling and living up to the protocol. Indeed, you did make your point there.
However, I’'m just wondering:
I found that the server simply does not return a result IQ packet when a client attempts to do this (which generally would cause clients to hang until the response times out)
What client are you using that waits for an IQ result? IMHO, except perhaps for the initial packet exchanges that starts with “<stream:stream …” and ends somewhere before “”, clients have to treat all stanzas in an asynchronous manner. They should not wait or anticipate what the next packet would be.
Smack does this all over the place for IQ packets since the general contract is that the server should always return a response for an IQ, regardless of success or failure. So any Smack client that tries to do these things would not receive an error IQ response, as specified in the JEP, but would time out instead.
Yes, this busy waiting on server response paradigm that goes through all of Smack made me not use most of the listener stuff that’'s in there. I just used a generic listener for IQ packets, and did the transaction handling myself, using Smack as a packet parser/generator.
Even worse is that some things (like the roster stuff) can’‘t be properly implemented in a Smack-using client without using a busy waiting loop. In these cases I had to patch those problems (I posted them to this forum, but some were rejected and still aren’'t fixed). These issues got me to think about creating my own XMPP-library with a sane transaction model.
I would agree with many of your assertions in regards to the waiting on response model. Though at its heart smack tries to stress convience ot the end user. So, say I am using the roster and I goto add a user, my resonable expectation from doing so is that the user will be added, and, if not an exception would be thrown explaning to me what occured. This in essence I believe is the theory behind the “busy waiting” that is present throughout smack. I know we have discussed the roster initialiaztion issues in the past, but I am curious to know what other sumbling blocks you have encountered along the way?
We have been working on fundamental changes to the ways some things are handled in Smack to make them more intuitive to developers utilizing the library. I believe I am somewhat attuned to the issues faced using Smack as it is, as I have developed my own client and I am always interested in discussions pertaining to how things can be improved, and even more so in implementing those improvements!
vcard-handling is another place where this is an issue, and disco-support (which I’'ve done manually because of this, too). The policy should be to never block on anything but waiting for the socket write (which should be near-instant anyways). You can use callbacks instead (inner classes etc).
On Mac OS X, the main thread is the most important one. When you’'re blocking the main thread, the whole GUI is blocked. This means that waiting on the server reply results in a sluggish application, which is very annoying.
The contract of IQ must be followed, as in rfc3920 excerpt below:
An entity that receives an IQ request of type “get” or “set” MUST reply with an IQ response of type “result” or “error” (the response MUST preserve the ‘‘id’’ attribute of the request).
However, that certainly doesn’'t imply that clients must wait buy not processing other stanzas. Another excerpt says:
The data content of the request and response is defined by the namespace declaration of a direct child element of the IQ element, and the interaction is tracked by the requesting entity through use of the ‘‘id’’ attribute.
Now that is the control key for the client: the ‘‘id’’ attribute. If an IQ interaction is to be displayed on a window, that window has to be modeless and have at least the “Hide” and “Cancel” button. Using the ‘‘id’’ the client could then decide what to do if all of a sudden an IQ result/error with that ‘‘id’’ comes back.
If Smack tries to provide an interface intuitive to developers and convenient to users, it should do that on that basis.
I also agree with anlumo. I respect the attempt to make Smack easier to use and maybe even abstract some of the details of XMPP away from developers (although I don’‘t know if that was the intent), but I think it’'s misguided in this instance. If Smack got away from a busy wait model and forced developers to use callbacks and make their code more asynchronous, it might make the initial development effort a bit harder, but would significantly lower time on the back end of the cycle in terms of debugging/bug fixing.
The busy wait model has its uses; in particular it certainly makes it much easier to throw together a simple test app. In the end though, it’'s probably not a good trade-off.
I am not sure I understand the added benefit of using callbacks as opposed to using the current callback model. Do you envision having a packet collector like object where you can peek or wait for a response from the server? In the end, you want the response from the server, the main argument that has been presented against using the busy waiting is that it causes your main thread to wait. So, my initial response to this is to say then dont use your main thread for server calls. If you are using swing you shouldnt be doing this anyway, as any tasks should be passed off to a worker thread, when creating my client I put Swingworker to extensive work when doing calls to the server. If you are not creating an end user client then you may consider using a pool of worker threads to handle these tasks.
THe only thread that is waiting is the thread that you used to call the method, in the mean time Smack is continuing to process other packets that it is recieving and forwarding those to listener’'s, etc. This is one area I could perhaps envision improvement as say you recieve packet x and want to execute action y, the first thing you may think to do is call do action y which, if you are using listeners, blocks any other listeners from being processed. Smack cannot use several threads to continue processing other messages as this may violate the contract of processing messages in the order that they are recieved and would lead to unpredicatable behavior. One possiblity then is to use several threads to process each packet, as there is no order execution expected when processing collectors and listeners.
In other ways I can see how the current model can be improved in certain ways, through improved developer documentation and some tweaks but I still cannot see the benefit of using callbacks as opposed to the waiting model that is currently used. Maybe some concrete code samples are in order of where a callback model would be better than the waiting model?
Of course, using the waiting model is easier to use. However, you’'re effectively proposing using a single thread for every request/reply action. This causes a huge overhead, and introduces many potential pitfalls for not multithreading-trained programmers (you might remember that Wildfire and Smack themselves contained or still contain many issues that only occur sometimes. This is caused by going massively multithreaded.).
Hmmm… I question the huge overhead needed for multithreaded programming. Executors, effectivly negate any overhead generated my using several threads to execute several server calls they also allow thread reuse and one can cap the number of threads in use, any tasks that come in without a thread open will be exectuted once a thread is available.
Your point of difficulty for the developer is noted and I think this is something that we would want to concentrate on fixing. Working with Swing or SWT, you are inherently working in a multithreaded environment, Swing tries to hide this fact and I think in effect does the developer more harm than good as long executions on the event loop result in a poor end user experience as it appears as though the interface is frozen.
In the call back mechanism the onerous is on the developer to track server calls that are awating a response. How do you handle these? Do you have a loop that is checking to see if the server has yet responded to these calls yet, then do you store some sort of auxillary information along with it to tell this thread how to handle particular responses? I fail again to see how this is any less confusing or difficult for the developer than using multithreaded development.
No, what you’'re describing is busy waiting yet again. In the callback model, you supply a method along with sending the IQ packet, the call returns immediately after sending the packet, and as soon as the reply is received, the supplied method is called. In Java, you could handle this by using an anonymous inner class.
IQ myIQ = new IQ();
// do something with myIQ... connection.sendIQPacket(myIQ, new IQCallback() {
void replyReceived(IQ reply) {
System.err.println("I got a reply!");
}
});