Respond to all incoming Socks5 bytestream requests
Description
Environment
Attachments
is related to
Activity
Henning Staib August 15, 2010 at 12:16 PM
done in revision 11821
Henning Staib March 22, 2010 at 3:23 PM
Wrong fromatting for package structure
smackx
bytestream
socks5
Socks5Manager
...
ibb
IBBManager
...
Henning Staib March 22, 2010 at 3:16 PM
Hi, thanks for the review.
I introduced the InitiationListener class just for clearness. It could also be a private inner class of the Socks5BytestreamManager. I prefer more small files more than a few big ones. The Socks5BytestreamManager should not directly implement a PacketListener because the #processPacket() method is required to be public but doesn't belong to the public API of the manager. But I'm ok with putting the class into the Socks5BytestreamManager.
The ErrorIQ class just exists because I just wanted to modify the existing Smack code as little as possible to lower the hurdle to commit it to the trunk. Your solution is much better and I will change the code once your patch is commited.
The memory leak can be solved by adding a ConnectionListener and remove the Socks5BytestreamManager along with the InitiationListener if the connection is closed.
I didn't knew about the separation of the different test case types until I wrote the guideline document. Will change that.
I'm also not very happy with the naming but I didn't want to use names that may conflict with future improvements. I think the API for In-Band Bytestreams should also be extracted one day and then the name bytestream is too unspecific. Same goes for the class names.
Maybe the packages could look like this:
smackx
bytestream
socks5
Socks5Manager
...
ibb
IBBManager
...
Do you know of a good way how to patch patches so that I can include your requests? I would prefer that I collect all your changes and create one new big patch once you are done with the other patches concernig Socks5.
Guenther Niess March 22, 2010 at 12:09 PM
First of all thanks for your work! I like that you introduce a new package for socks5 bytestreams
I've one design question. Why do you introduce the InitiationListener class?
In my opinion this class seems to be closely related to Socks5ByteStreamManager. Maybe it make sense to integrate it and to have all related functions wihtin one class?
I know more packages and classes use their own implementation to create error or empty IQ result packages, but in my opinion we should instead
improve the IQ API to create empty IQ results or IQ errors and reuse it. So I would suggest to remove ErrorIQ because it isn't bytestream specific. I've created to reflect this.
I think I've found a memory leak for Socks5ByteStreamManager if a implementation creates many connections (and for each connection a ByteStreamManager is created but never removed if the connection is closed).
Use the test folder for integration testing against an Openfire server and the test-unit directory for unit tests.
I would favour bytestream as package name instead of socks5bytestream and ByteStreamManager instead of Socks5ByteStreamManager but maybe thats only my taste
XEP-0065 (http://xmpp.org/extensions/xep-0065.html#proto-inform) says if client supports Socks5 bytestream it MUST answer to Socks5 bytestream requests by either accepting or rejecting it.
added Socks5BytestreamManager to startup classes in smack-config.xml
Socks5BytestreamManager initializes Socks5 bytestream feature and InitiationListener that listens for all incoming Socks5 bytestream requests
Socks5BytestreamManager has methods to add or remove IncomingByteStreamListener that will be notified on incoming requests
InitiationListener rejects all requests if no IncomingByteStreamListener are registered
if IncomingByteStreamListener are registered a Socks5BytestreamRequest is created and passed to the listeners
ErrorIQ class for convenience to reply to a packet with an error (there was no IQ subclass with no child elements)
contains tests for ErrorIQ and InitiationListener