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

SockJS Support for Netty4 #1615

Closed
wants to merge 71 commits into
base: master
from

Conversation

Projects
None yet
@danbev
Member

danbev commented Jul 19, 2013

Please see sockjs/README.md for instructions about running tests general information regarding the SockJS support.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 19, 2013

Build result for #1615 at 8388bc4703902a32222b3ed4fe885ae2eec4df75: Success

ghost commented Jul 19, 2013

Build result for #1615 at 8388bc4703902a32222b3ed4fe885ae2eec4df75: Success

@ghost ghost assigned normanmaurer Jul 19, 2013

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jul 19, 2013

Member

@danbev YAY ... Let me review it... Thanks also for signing the CLA :)

Member

normanmaurer commented Jul 19, 2013

@danbev YAY ... Let me review it... Thanks also for signing the CLA :)

@normanmaurer

View changes

Show outdated Hide outdated sockjs/src/main/java/io/netty/handler/codec/sockjs/Config.java
@normanmaurer

View changes

Show outdated Hide outdated sockjs/src/main/java/io/netty/handler/codec/sockjs/Config.java
@normanmaurer

View changes

Show outdated Hide outdated sockjs/src/main/java/io/netty/handler/codec/sockjs/Config.java
@normanmaurer

View changes

Show outdated Hide outdated sockjs/src/main/java/io/netty/handler/codec/sockjs/Config.java
@normanmaurer

View changes

Show outdated Hide outdated ...main/java/io/netty/handler/codec/sockjs/handlers/CorsInboundHandler.java
@normanmaurer

View changes

Show outdated Hide outdated ...main/java/io/netty/handler/codec/sockjs/handlers/CorsInboundHandler.java
@normanmaurer

View changes

Show outdated Hide outdated ...main/java/io/netty/handler/codec/sockjs/handlers/CorsInboundHandler.java
@normanmaurer

View changes

Show outdated Hide outdated ...s/src/main/java/io/netty/handler/codec/sockjs/handlers/CorsMetadata.java
@normanmaurer

View changes

Show outdated Hide outdated ...ain/java/io/netty/handler/codec/sockjs/handlers/PollingSessionState.java
@normanmaurer

View changes

Show outdated Hide outdated ...ain/java/io/netty/handler/codec/sockjs/handlers/PollingSessionState.java
@normanmaurer

View changes

Show outdated Hide outdated ...ain/java/io/netty/handler/codec/sockjs/handlers/PollingSessionState.java
@normanmaurer

View changes

Show outdated Hide outdated ...ain/java/io/netty/handler/codec/sockjs/handlers/PollingSessionState.java
@normanmaurer

View changes

Show outdated Hide outdated ...ain/java/io/netty/handler/codec/sockjs/handlers/SendingSessionState.java
@normanmaurer

View changes

Show outdated Hide outdated ...src/main/java/io/netty/handler/codec/sockjs/handlers/SessionHandler.java
@normanmaurer

View changes

Show outdated Hide outdated ...src/main/java/io/netty/handler/codec/sockjs/handlers/SessionHandler.java
@normanmaurer

View changes

Show outdated Hide outdated ...s/src/main/java/io/netty/handler/codec/sockjs/handlers/SessionState.java
@normanmaurer

View changes

Show outdated Hide outdated .../src/main/java/io/netty/handler/codec/sockjs/handlers/SockJSHandler.java
@normanmaurer

View changes

Show outdated Hide outdated .../src/main/java/io/netty/handler/codec/sockjs/handlers/SockJSHandler.java
@normanmaurer

View changes

Show outdated Hide outdated .../src/main/java/io/netty/handler/codec/sockjs/handlers/SockJSHandler.java
@normanmaurer

View changes

Show outdated Hide outdated .../src/main/java/io/netty/handler/codec/sockjs/handlers/SockJSSession.java
@normanmaurer

View changes

Show outdated Hide outdated ...n/java/io/netty/handler/codec/sockjs/handlers/StreamingSessionState.java
@normanmaurer

View changes

Show outdated Hide outdated ...n/java/io/netty/handler/codec/sockjs/handlers/StreamingSessionState.java
@normanmaurer

View changes

Show outdated Hide outdated sockjs/src/main/java/io/netty/handler/codec/sockjs/protocol/CloseFrame.java
@normanmaurer

View changes

Show outdated Hide outdated sockjs/src/main/java/io/netty/handler/codec/sockjs/protocol/CloseFrame.java
@normanmaurer

View changes

Show outdated Hide outdated sockjs/src/main/java/io/netty/handler/codec/sockjs/protocol/Frame.java
@normanmaurer

View changes

Show outdated Hide outdated sockjs/src/main/java/io/netty/handler/codec/sockjs/protocol/CloseFrame.java
@normanmaurer

View changes

Show outdated Hide outdated sockjs/src/main/java/io/netty/handler/codec/sockjs/protocol/Greeting.java
@normanmaurer

View changes

Show outdated Hide outdated ...src/main/java/io/netty/handler/codec/sockjs/protocol/HeartbeatFrame.java
@normanmaurer

View changes

Show outdated Hide outdated ...s/src/main/java/io/netty/handler/codec/sockjs/protocol/MessageFrame.java
@normanmaurer

View changes

Show outdated Hide outdated ...s/src/main/java/io/netty/handler/codec/sockjs/protocol/MessageFrame.java
@normanmaurer

View changes

Show outdated Hide outdated sockjs/src/main/java/io/netty/handler/codec/sockjs/protocol/OpenFrame.java
@normanmaurer

View changes

Show outdated Hide outdated ...s/src/main/java/io/netty/handler/codec/sockjs/protocol/PreludeFrame.java
@normanmaurer

View changes

Show outdated Hide outdated ...java/io/netty/handler/codec/sockjs/transports/AbstractSendTransport.java
@normanmaurer

View changes

Show outdated Hide outdated ...java/io/netty/handler/codec/sockjs/transports/AbstractSendTransport.java
@normanmaurer

View changes

Show outdated Hide outdated ...java/io/netty/handler/codec/sockjs/transports/AbstractSendTransport.java
@normanmaurer

View changes

Show outdated Hide outdated .../java/io/netty/handler/codec/sockjs/transports/EventSourceTransport.java
@normanmaurer

View changes

Show outdated Hide outdated .../java/io/netty/handler/codec/sockjs/transports/EventSourceTransport.java
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 18, 2014

Build result for #1615 at a4ed84e: Success

ghost commented Mar 18, 2014

Build result for #1615 at a4ed84e: Success

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Mar 23, 2014

Member

@danbev please let you know once you think it is "complete"

Member

normanmaurer commented Mar 23, 2014

@danbev please let you know once you think it is "complete"

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev Mar 23, 2014

Member

@normanmaurer will do. I'm currently taking another stab at the refactoring to avoid the API on top of Netty. Made some progress on Friday and hope to continue tomorrow.

Member

danbev commented Mar 23, 2014

@normanmaurer will do. I'm currently taking another stab at the refactoring to avoid the API on top of Netty. Made some progress on Friday and hope to continue tomorrow.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 11, 2014

Build result for #1615 at a4ed84e: Success

ghost commented May 11, 2014

Build result for #1615 at a4ed84e: Success

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Sep 23, 2014

Member

@normanmaurer @danbev - I'm going to move this to 5.0.0.Alpha3 (outstanding for a while, needs to absorb latest changes from master, not squashed). Let me know if you have any objections.

Member

Scottmitch commented Sep 23, 2014

@normanmaurer @danbev - I'm going to move this to 5.0.0.Alpha3 (outstanding for a while, needs to absorb latest changes from master, not squashed). Let me know if you have any objections.

@Scottmitch Scottmitch added the feature label Sep 23, 2014

@Scottmitch Scottmitch modified the milestones: 5.0.0.Alpha3, 5.0.0.Alpha2 Sep 23, 2014

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev Sep 24, 2014

Member

@Scottmitch No problems. I've got this on my list to update with the latest changes from master and then go through it once more with @normanmaurer.

Member

danbev commented Sep 24, 2014

@Scottmitch No problems. I've got this on my list to update with the latest changes from master and then go through it once more with @normanmaurer.

@chemist777

This comment has been minimized.

Show comment
Hide comment
@chemist777

chemist777 Sep 25, 2014

@danbev HeartbeatTimer for jsonp-polling transport does not work. Because condition ctx.channel().isActive() && ctx.channel().isRegistered() in the file io/netty/handler/codec/sockjs/handler/AbstractTimersSessionState.java is always false for JSONP. Browser sockjs client with simple reconnection logic to echo server quickly reaches his HTTP connections limit.

chemist777 commented Sep 25, 2014

@danbev HeartbeatTimer for jsonp-polling transport does not work. Because condition ctx.channel().isActive() && ctx.channel().isRegistered() in the file io/netty/handler/codec/sockjs/handler/AbstractTimersSessionState.java is always false for JSONP. Browser sockjs client with simple reconnection logic to echo server quickly reaches his HTTP connections limit.

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev Sep 25, 2014

Member

@chatovod Thanks for reporting this! I'm going to revisit this PR after next week (on holidays) and I'll make sure I cover this case.

Member

danbev commented Sep 25, 2014

@chatovod Thanks for reporting this! I'm going to revisit this PR after next week (on holidays) and I'll make sure I cover this case.

@chemist777

This comment has been minimized.

Show comment
Hide comment
@chemist777

chemist777 Sep 25, 2014

@danbev One more bug. SockJsSession will never be removed from server sessions in case of session timeout because you use TimeUnit.NANOSECONDS.toMillis(System.nanoTime()). It is not always the same as System.currentTimeMillis() in Oracle JDK 8 64 bit. See the AbstractTimersSessionState.java.

And third bug.
io.netty.handler.codec.sockjs.SockJsService#onClose will never be executed in case of session timeout.

chemist777 commented Sep 25, 2014

@danbev One more bug. SockJsSession will never be removed from server sessions in case of session timeout because you use TimeUnit.NANOSECONDS.toMillis(System.nanoTime()). It is not always the same as System.currentTimeMillis() in Oracle JDK 8 64 bit. See the AbstractTimersSessionState.java.

And third bug.
io.netty.handler.codec.sockjs.SockJsService#onClose will never be executed in case of session timeout.

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev Sep 25, 2014

Member

@chatovod Great, (well, not great that there are bugs but...) I'll take a look at this one too. Thanks!

Member

danbev commented Sep 25, 2014

@chatovod Great, (well, not great that there are bugs but...) I'll take a look at this one too. Thanks!

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev Sep 26, 2014

Member

When I get back I'll also update the default SockJS CDN as this is being changed. More information can be found in this issue.

Member

danbev commented Sep 26, 2014

When I get back I'll also update the default SockJS CDN as this is being changed. More information can be found in this issue.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 13, 2014

Build result for #1615 at a4ed84e: Unstable

ghost commented Oct 13, 2014

Build result for #1615 at a4ed84e: Unstable

@trustin

This comment has been minimized.

Show comment
Hide comment
@trustin

trustin Oct 13, 2014

Member

@danbev, first of all, thank you so much for your patience on this pull request.

Initially, I thought it is better implementing this as a meta-transport, but I ended up thinking that it might lead to just another abstraction leak.

Therefore, I'd like to ask you to:

  • forget about my idea about meta-transport,
  • create a new pull request which is same with this one, so that we don't suffer with the loading time of this page, and
  • close this pull request and add a link to the new pull request to continue your work.

Once you're done, I'll do an extensive review so that this can be merged into upstream.

Member

trustin commented Oct 13, 2014

@danbev, first of all, thank you so much for your patience on this pull request.

Initially, I thought it is better implementing this as a meta-transport, but I ended up thinking that it might lead to just another abstraction leak.

Therefore, I'd like to ask you to:

  • forget about my idea about meta-transport,
  • create a new pull request which is same with this one, so that we don't suffer with the loading time of this page, and
  • close this pull request and add a link to the new pull request to continue your work.

Once you're done, I'll do an extensive review so that this can be merged into upstream.

@trustin

This comment has been minimized.

Show comment
Hide comment
@trustin

trustin Oct 13, 2014

Member

Oh, and you might want to rebase against 4.1, because we can make this part of 4.1 for sure.

Member

trustin commented Oct 13, 2014

Oh, and you might want to rebase against 4.1, because we can make this part of 4.1 for sure.

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev Oct 13, 2014

Member

Initially, I thought it is better implementing this as a meta-transport, but I ended up thinking that it might >lead to just another abstraction leak.

I have attempted to create such a meta-transport which I've managed to get working but I'm not really sure about it. Would be great if you can take a look and see what you think of it. If you still think it's a bad idea, I'll take care of the 3 bugs reported here by @chemist777 and create a new rebased PR.

Member

danbev commented Oct 13, 2014

Initially, I thought it is better implementing this as a meta-transport, but I ended up thinking that it might >lead to just another abstraction leak.

I have attempted to create such a meta-transport which I've managed to get working but I'm not really sure about it. Would be great if you can take a look and see what you think of it. If you still think it's a bad idea, I'll take care of the 3 bugs reported here by @chemist777 and create a new rebased PR.

@trustin

This comment has been minimized.

Show comment
Hide comment
@trustin

trustin Oct 13, 2014

Member

@danbev You are an expert on SockJS and I'm not, so I'd say you should tell me which sounds better to you and why. If there's no good reason to keep the meta-transport and it has more shortcomings than this pull request, we should keep this pull request. Let me know what you think.

Member

trustin commented Oct 13, 2014

@danbev You are an expert on SockJS and I'm not, so I'd say you should tell me which sounds better to you and why. If there's no good reason to keep the meta-transport and it has more shortcomings than this pull request, we should keep this pull request. Let me know what you think.

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev Oct 14, 2014

Member

@trustin What I liked with your suggestion was that is uses Netty constructs, you just have a normal ChannelHandler to implement and you simply use the normal API to configure, and write messages etc.

But while implementing I came across issues and it felt like I'm "shoe horning" this into Netty. My concern is that it SockJS might not be a good choice for such a meta-transport as it is really a protocol on top of HTTP in a simliar way to WebSocket. I some ways it is similar to WebSocketServerProtocolHandler.
I'll rebase and create a new PR and have it ready for review. As there have been so many commits would you prefer that I squash them all?

Member

danbev commented Oct 14, 2014

@trustin What I liked with your suggestion was that is uses Netty constructs, you just have a normal ChannelHandler to implement and you simply use the normal API to configure, and write messages etc.

But while implementing I came across issues and it felt like I'm "shoe horning" this into Netty. My concern is that it SockJS might not be a good choice for such a meta-transport as it is really a protocol on top of HTTP in a simliar way to WebSocket. I some ways it is similar to WebSocketServerProtocolHandler.
I'll rebase and create a new PR and have it ready for review. As there have been so many commits would you prefer that I squash them all?

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 14, 2014

Member

Yes please squash

Am 14.10.2014 um 08:33 schrieb Daniel Bevenius notifications@github.com:

@trustin What I liked with your suggestion was that is uses Netty constructs, you just have a normal ChannelHandler to implement and you simply use the normal API to configure, and write messages etc.

But while implementing I came across issues and it felt like I'm "shoe horning" this into Netty. My concern is that it SockJS might not be a good choice for such a meta-transport as it is really a protocol on top of HTTP in a simliar way to WebSocket. I some ways it is similar to WebSocketServerProtocolHandler.
I'll rebase and create a new PR and have it ready for review. As there have been so many commits would you prefer that I squash them all?


Reply to this email directly or view it on GitHub.

Member

normanmaurer commented Oct 14, 2014

Yes please squash

Am 14.10.2014 um 08:33 schrieb Daniel Bevenius notifications@github.com:

@trustin What I liked with your suggestion was that is uses Netty constructs, you just have a normal ChannelHandler to implement and you simply use the normal API to configure, and write messages etc.

But while implementing I came across issues and it felt like I'm "shoe horning" this into Netty. My concern is that it SockJS might not be a good choice for such a meta-transport as it is really a protocol on top of HTTP in a simliar way to WebSocket. I some ways it is similar to WebSocketServerProtocolHandler.
I'll rebase and create a new PR and have it ready for review. As there have been so many commits would you prefer that I squash them all?


Reply to this email directly or view it on GitHub.

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev Oct 23, 2014

Member

Closing this pull request in favour of #3045

Member

danbev commented Oct 23, 2014

Closing this pull request in favour of #3045

@danbev danbev closed this Oct 23, 2014

@danbev danbev deleted the danbev:sockjs branch Jan 20, 2015

@mortan

This comment has been minimized.

Show comment
Hide comment
@mortan

mortan Jan 25, 2015

Any update on this? Would love sock.js support in netty! 👍

mortan commented Jan 25, 2015

Any update on this? Would love sock.js support in netty! 👍

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev Jan 26, 2015

Member

@mortan I'm currently working on getting #3271 merged and after that I'll rebase and hopefully a final round of reviews before merging. Can't give an exact date but there should be some updates to this PR shortly, hopefully later this week or next.

Member

danbev commented Jan 26, 2015

@mortan I'm currently working on getting #3271 merged and after that I'll rebase and hopefully a final round of reviews before merging. Can't give an exact date but there should be some updates to this PR shortly, hopefully later this week or next.

@danbev

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev Mar 5, 2015

Member

This needs to be rebased against the latest 4.1 version and verify that
things are working correctly. The last rebase passed the
sockjs-protocol-0.3.3 but only after custom updates due to how header
values are compared in the test, so nothing major. I've not got much time
at the moment due to parental leave but will try to rebase and run the
sockjs-protocol tests this weekend and also update the PR against
sockjs-protocol (the header issue mentioned above)

onsdag 4 mars 2015 skrev Tiger Toes notifications@github.com:

Hey @danbev https://github.com/danbev, is there any update to this? Is
there anything I can do to help?


Reply to this email directly or view it on GitHub
#1615 (comment).

Member

danbev commented Mar 5, 2015

This needs to be rebased against the latest 4.1 version and verify that
things are working correctly. The last rebase passed the
sockjs-protocol-0.3.3 but only after custom updates due to how header
values are compared in the test, so nothing major. I've not got much time
at the moment due to parental leave but will try to rebase and run the
sockjs-protocol tests this weekend and also update the PR against
sockjs-protocol (the header issue mentioned above)

onsdag 4 mars 2015 skrev Tiger Toes notifications@github.com:

Hey @danbev https://github.com/danbev, is there any update to this? Is
there anything I can do to help?


Reply to this email directly or view it on GitHub
#1615 (comment).

@mortan

This comment has been minimized.

Show comment
Hide comment
@mortan

mortan Apr 10, 2015

Still sitting on the edge of my seat and waiting for it! :)

mortan commented Apr 10, 2015

Still sitting on the edge of my seat and waiting for it! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment