Respond to all incoming Socks5 bytestream requests

Description

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

Environment

None

Activity

Show:
Henning Staib
August 15, 2010, 12:16 PM

done in revision 11821

Henning Staib
March 22, 2010, 3:23 PM

Wrong fromatting for package structure

  • smackx

    • bytestream

      • socks5

        • Socks5Manager

        • ...

      • ibb

        • IBBManager

        • ...

Henning Staib
March 22, 2010, 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, 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

Fixed
Your pinned fields
Click on the next to a field label to start pinning.

Assignee

Robin Collier

Reporter

Henning Staib