-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SOCKS proxy support (attempt #2) #486
Conversation
what do you guys think about creating new |
I'm generally a fan of separate *Connection classes that implement different functionality, rather than branching. Ideally if they're composeable somehow and maybe you could have a |
SOCKS proxies are fairly different from HTTP proxies. They operate at a lower level. Separate classes are not strictly necessary, but I think the design makes sense. The only bottleneck for me in finishing this was getting the tests to work. Finding decent SOCKS4 and SOCKS5 daemons that worked with Tornado was a lot more difficult than I imagined. I also had all sorts of indeterministic-seeming exceptions that seemed to stem from conflicts with Tornado's and Twisted's event loops. I eventually gave up, though I think some changes were later made to the Tornado test cases that could have resolved the issues I was seeing. If you can help set up the test servers I can rewrite the original tests (and add new ones) and ensure the code is working as intended, on top of making the integration as clean and idiomatic as possible. I think my last commit was decent but it'll need serious modification to account for some changes in Edit Actually, I might have a better idea for integrating the support. I'd appreciate it if you left this commit as it is for now (excluding any work on the tests you may be doing) while I work on a new PR. |
I'm not familiar with the SOCKS protocol but FWIW we've been trying to use socket-level tests wherever possible. It may be impractical to implement subsets of a SOCKS server like this, though. It may be more feasible to find a python socks lib which implements enough of the protocol to use ad-hoc like this. Building another test suite alongside our Tornado-based one is brutally painful, especially using Twisted (we had sooo many problems with Twisted way back when). Note to self: Break up socket-level tests into submodules. |
@Anorov okay will leave everything else alone. Hmm on second reading |
I put a lot of work to make the tornado tests more stable and self explaining. Mainly: So it's very likely you have seen one of those issues and you should definitely give it a try again. Feel free to ping me if you run in another issue. |
@Anorov any word on this, or link to a new PR? |
@bpowers I have an awful habit of writing code while neglecting to finish the tests. I have a working version with what I feel is a better design, I'll submit a PR within a week or two. |
@Anorov I invite you to open a PR early and often. :) If it's not ready, just add [WIP] to the title, and we're happy to watch it evolve (and maybe give some help as it goes along). Worst case, somebody might find it useful to bootstrap their own PR later. |
We do need the SOCKS proxy support! |
@zhao-ji Lots of people need the SOCKS proxy support, but relatively few seem to need it enough to put the work in to build it. This is at least the second pull request for this function that has stalled. =( If you're prepared to do the work, we'll be happy to help get it into a shape that we can merge it! |
how is urllib3 SOCKS support coming? |
Hello, I have $100 USD that I'm interested in putting toward SOCKS support bounties. In particular, the features I'm interested in are:
I'm not sure if $100 is sufficient to achieve all of the above, or what the distribution would be between those features. If a urllib3 developer can advise on how to split up the $100 between those task bounties (and on whether a bounty would even be helpful for getting this done faster), I'll happily create the relevant GitHub issues and post the bounties on BountySource. Cheers. |
@JeremyRand That's a great idea, thank you. Now that you mention it, we really should open up an umbrella issue (or a couple) to track SOCKS support that isn't a PR. Would you like to put that together? Ideally reference all the related attempts we've had so far. In terms of splitting up the bounty, I would advise to put it all towards getting somekind of baseline SOCKS proxy support to start. It's a fairly massive task that has been stalled for a years now, so I think we should put all of our energy/finances into incentivizing the baseline to be completed before we worry about adding specific features to it. $100 is a great start, but I suspect we'll need to raise more funds to justify somebody spending a few of weeks working on this ~full time. Once we have an umbrella issue, it would be really helpful to try and rally more people towards kickstarting the bounty towards that issue (maybe by reaching out to all the people who pinged the issue over the years, or finding corporate sponsors). I can work on this with you too. :) |
Actually I'll start the issue here: #690 |
@JeremyRand I'm not interested in the bounty, but between the PySocks library I maintain and my PR, the requirements for 1-4 have been met. Once test code has been added, it should work with no changes needed in Requests. Usage example: Please split the bounty between the other contributors, particularly anyone who helps with tests. |
Obsoleted by #762 |
initial shot at revitalizing #284
continuation of work from discussion at #68 -- going to see about using pycurl + Tornado for tests.