http_proxy support about gniazdo HOT 23 CLOSED

@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.

from gniazdo.

@jakubholynet The repository is now available at:

https://github.com/stylefruits/gniazdo-jsr356

I already sent you an invitation to become a collaborator. :)

from gniazdo.

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!

from gniazdo.

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 :(

from gniazdo.

The upstream bug 409516 was moved to jetty/jetty.project#117

from gniazdo.

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?

from gniazdo.

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

from gniazdo.

@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 client passed to connect, the session in on-connect, the type of the Sendable's parameter) are different and have different APIs so this would be a breaking change for users that use them.

(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!

from gniazdo.

@jakubholynet Thanks a lot for the patch! We'll review it next week; I hope that's not a problem.

from gniazdo.

Ok!! Thanks!
fre. 12. aug. 2016 kl. 14.34 skrev Jan Stępień [email protected]:

@jakubholynet https://github.com/jakubholynet Thanks a lot for the
patch! We'll review it next week; I hope that's not a problem.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAmJPoCODINFCiKg9bSKaniZ3vSY9X-sks5qfGhNgaJpZM4Gn1NK
.

from gniazdo.

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!

from gniazdo.

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:

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.

I guess you do not mean the two essentially empty *MessageHandlers but the two classes copied from Jetty. In general you are right but not in this case. It is quite unlikely that the format of the sec-websocket-extensions header will change and thus the classes can stay as they are forever. Alternatively, we could depend on jetty and use jetty's.

We're also worried about the “reflection magic” you mentioned in comments.
I wouldn't be too worried if you are concerned about performance. The overhead is only incurred upon "connect", when the message handlers are registered. During message sending and receiving, this reflection is not involved.

I will use my forked Gniazdo to get a Slack bot running in our enterprise network so it wasn't completely wasted effort.

from gniazdo.

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 javax.websocket API. So you could easily swap in whatever javax.websocket implementation you wanted - whether Tyrus or Jetty (yes, there is a Jetty-based implementation of javax.websocket)

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!)

from gniazdo.

@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 javax.websocket.* support) is worth the maintenance effort it incurs.

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!

from gniazdo.

OK!

Would it be possible to have the fork under the stylefruits org? I would name it gniazdo-jsr356 unless somebody has a better idea.

from gniazdo.

I have now renamed my fork and adjusted the Readme accordingly: https://github.com/jakubholynet/gniazdo-jsr356

from gniazdo.

@xsc What do you think about hosting gniazdo-jsr356 under stylefruits?

from gniazdo.

@xsc, @jstepien What do you think about hosting gniazdo-jsr356 under stylefruits?

from gniazdo.

@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.)

from gniazdo.

Great, thanks! Yes, I will.

On Mon, Oct 31, 2016 at 4:43 PM Yannick Scherer [email protected]
wrote:

@jakubholynet https://github.com/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.)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAmJPopXx1-F74cWeaWhJju4KJULBRmCks5q5gxagaJpZM4Gn1NK
.

from gniazdo.

I have tried it to transfer but cannot. From the transfer page:

Transfer this repository to another user or to an organization where you have admin rights.
Any ideas? Perhaps you can just clone https://github.com/jakubholynet/gniazdo-jsr356 (or https://github.com/jakubholynet/gniazdo-jsr356-forked - both have the same code but the forked one complained about your account already having a fork in the network) and push to your org and make me a collaborator?

Thanks!

from gniazdo.

Accepted, thanks!

Regarding

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.

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 Gniazdo

As 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).

from gniazdo.

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).

from gniazdo.