-
Notifications
You must be signed in to change notification settings - Fork 38
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
http_proxy support #16
Comments
No, not yet. It'd be great to have it but we're blocked by the upstream bug 409516. Once this obstacle is gone we'd love to see a pull request adding proxy support! |
Looks like the jetty guys are moving incredibly slow 👴. I wonder about switching to Tyrus as the websocket client, which is the reference implementation for websockets in Java: https://tyrus.java.net/documentation/1.9/user-guide.html#d0e1323 Probably not a trivial change :( |
The upstream bug 409516 was moved to jetty/jetty.project#117 |
I was thinking about trying to replace Jetty with Tyrus, just to give it a try to see how much work would that be. Would it be interesting to you? |
@jakubholynet Sounds good! We'll have to maintain the public API, though, so I guess the feasibility of this stands and falls with how compatible Jetty's and Tyrus' underlying concepts are. |
FYI I am working on this and am ± finished code-wise, only need to do proper automated and manual testing. For an early preview, check out https://github.com/jakubholynet/gniazdo/blob/tyrus-replaces-jetty/src/gniazdo/core.clj |
@xsc & Co.: I would now really appreciate feedback on the integration of Tyrus (see https://github.com/jakubholynet/gniazdo/blob/tyrus-replaces-jetty/src/gniazdo/core.clj) The gnizado's API is preserved but of course the underlying classes (the (BTW I still need to clean up the commit history, comments, and perhaps the code itself slightly and try it in a production application but the code should be 100% finished.) Thank you! |
@jakubholynet Thanks a lot for the patch! We'll review it next week; I hope that's not a problem. |
Ok!! Thanks!
|
Hi Jakub, I'm writing on my and @xsc's behalf. We took a look at the overall diff and we have to admit that it's quite a massive change. All the newly added Java files will incur a lot of extra maintenance effort in order to keep them up to date with upstream changes and patches. We're also worried about the “reflection magic” you mentioned in comments. We try to keep Gniazdo free from reflective calls. Since Tyrus' WebSocket implementation uses reflections internally, we're reluctant to use it as our foundation. Extra CPU cost of reflective calls is of a secondary importance. Our main concern is that using reflective calls in Java is an indicator of some design or implementation issues swiped under the rug. Finally, as you noted above, the patch breaks backwards compatibility. We'd rather avoid it. Overall, it looks to us that adding support for HTTP proxies with this patch comes at a really high cost. A high cost primarily in terms of maintenance effort and introduced complexity. We hope you understand our doubts. We'd rather patiently wait for our friends maintaining Jetty to add proxy support to their client. Maybe you'd like to help them out? Thanks for taking time to work on this issue! |
I respect your decision, there is nothing that can be done about the "backwards compatibility" (if Jetty classes are considered part of that contract). Of course this is a massive change, since more less all that Gniazdo does is that it wraps Jetty and that has been replaced with something else :-) But I disagree with some of the comments:
I guess you do not mean the two essentially empty
I will use my forked Gniazdo to get a Slack bot running in our enterprise network so it wasn't completely wasted effort. |
Comment #2: You might actually consider accepting most of this change in somewhat modified form anyway - because the code isn't really Tyrus specific, it (now) uses exclusively the May be this is the direction Gniazdo should move in, ie. relying on the standard javax.websocket API instead of the proprietary Jetty one? Then users can themselves decide which implementation they want to use. If you do not think that it is a good idea then I would like to create - with your blessing a fork of Gniazdo that does this. (Name proposals welcome!) |
@jakubholynet We thought about this topic a bit more and came to the conclusion that gniazdo should just be a wrapper around the Jetty websocket implementation. Mainly, we're not sure that adding another layer of abstraction (in the form of generic That said, it would be waste for your work to be for naught, so feel free to fork gniazdo and apply your changes. We'll be happy to link to the new repository from our README. Again, thanks for your efforts! |
OK! Would it be possible to have the fork under the |
I have now renamed my fork and adjusted the Readme accordingly: https://github.com/jakubholynet/gniazdo-jsr356 |
@xsc What do you think about hosting gniazdo-jsr356 under stylefruits? |
@jakubholynet I'm so sorry that this went unanswered – even more so since this topic was internally already discussed a few weeks ago! Due to an oversight on my part the result was never communicated. Again, so sorry! We can definitely put gniazdo-jsr356 under the stylefruits organisation but due to our unfamiliarity with the underlying Tyrus client we'll reserve the right to address PRs and issues on a case-to-case basis. If this is a compromise you can live with, just transfer ownership to the stylefruits organisation. (And again, I can't tell you how sorry and embarrassed I am about this.) |
Great, thanks! Yes, I will. On Mon, Oct 31, 2016 at 4:43 PM Yannick Scherer notifications@github.com
|
I have tried it to transfer but cannot. From the transfer page:
Thanks! |
@jakubholynet The repository is now available at: https://github.com/stylefruits/gniazdo-jsr356 I already sent you an invitation to become a collaborator. :) |
Accepted, thanks! Regarding
don't hesitate to ping me whenever there is an issue or a PR, I am not giving up my responsibility of a maintainer. (Though it is easy for me to overlook notifications from GitHub so it may take a while to react.) Also, if there is a fix in Gniazdo that is also relevant for the Tyrus version, let me know to replicate it there. Thanks! Current status of http_proxy support in GniazdoAs of 11/2016, it is not supported in gniazdo due to limitations of Jetty 9 (see jetty/jetty.project#117). It seems they plan to fix it in Jetty 9.4.x. However, you can use gniazdo-jsr356 instead of Gniazdo. It has outwardly the same API but internally it uses the standard Java EE websockets API and its Tyrus implementation (while also supporting Jetty). Tyrus does support http_proxy (and other settings). |
Further discussion should be done in #19, since this thread went a bit off topic (but I don't want to purge the existing comments). |
Is there any support for proxies?
The text was updated successfully, but these errors were encountered: