Opened 11 years ago

Last modified 7 years ago

#3508 enhancement new

Add a SOCKS client API

Reported by: gr0gmint Owned by: ashfall
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch:

Description (last modified by Jean-Paul Calderone)

It is often useful and occasionally necessary to run an existing protocol through a SOCKS proxy. Twisted should provide an API which is orthogonal to existing client factories and protocols which allows nearly arbitrary existing application code to communicate through such a proxy. Perhaps this could take the form of a WrappingFactory and ProtocolWrapper pair or the form of an endpoint (or a combination of the two).

Attachments (1) (14.7 KB) - added by gr0gmint 11 years ago.

Download all attachments as: .zip

Change History (6)

Changed 11 years ago by gr0gmint

Attachment: added

comment:1 Changed 11 years ago by teratorn

Thanks for your efforts! This comment is by no means a complete review... I just glanced at the code briefly...

  • Good test coverage using twisted.trial will need to be implemented
  • 4-space indents
  • camelCase your method names, put spaces after commas in method signatures, and follow other conventions of the Twisted Coding Standard
  • Error message strings should simply be used in-line instead of referencing indexes in a list.
  • There is a mass of redundant and duplicated code in your exception classes. Simply using inheritence and "pass" for the body suite should suffice.
  • Don't use a double-underscore. Just a single underscore for private member names.
  • Try to break up super-long methods in to a collection of smaller, more modular methods.

There are other issues, but that should keep you busy for a while, and I'm afraid I'm out of time right now :)

Before you do any extra work, however, I would recommend getting feedback on your architecture. I can imagine other forms that SOCKS client support would take. If noone else comments on your code in detail, within a few days, you may want to solicit feedback on IRC or the mailing list.

comment:2 Changed 11 years ago by Jean-Paul Calderone

Milestone: regular-releases

Hi! The regular-releases milestone has nothing to do with SOCKS. It's about tasks necessary to make the Twisted release process simpler. Please don't apply it to tickets that aren't related to releasing Twisted.

comment:3 Changed 10 years ago by Jean-Paul Calderone

Description: modified (diff)
Keywords: socks removed
Summary: SocksiPy port for TwistedAdd a SOCKS client API

Beyond the issues teratorn pointed out, there are a number of issues with the attached code:

  • It uses Python 2.5 features; Twisted currently supports Python 2.3 and Python 2.4 as well.
  • It doesn't really have any error handling; raising exceptions whenever a problem is encountered will lead to exceptions being logged and connections being dropped, and not call the right connection lost/failed callbacks.
  • The code writes to standard out.
  • The point of extension is very unclear. None of the existing mechanisms are used in this code to let applications take advantage of this functionality.

Taken all together, I think this suggests that the shape of the original SocksiPy code isn't really suitable as a base for something to be included in Twisted.

It would be great to have a good SOCKS client in Twisted, so I'm not closing this, but I am going to adjust it to reflect the real goal.

The original description was:

I made a port of SocksiPy for Twisted. AFAIK Twisted doesn't have a SOCKS client protocol yet, only server

comment:4 Changed 9 years ago by Glyph

Owner: changed from Glyph to gr0gmint

Reassigning to the original reporter, since this was effectively a code review.

comment:5 Changed 7 years ago by ashfall

Owner: changed from gr0gmint to ashfall
Priority: lownormal
Note: See TracTickets for help on using tickets.