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

Not throw an exception if subprotocol is not supported but just drop the... #2149

Closed
wants to merge 1 commit into
base: 4.0
from

Conversation

Projects
None yet
2 participants
@normanmaurer
Member

normanmaurer commented Jan 23, 2014

... header as stated in the RFC's

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 23, 2014

Member

The problem is that at the moment we throw an WebSocketHandshakeException on the server-side. The proper way would be to just not send back the sub-protocol header like stated in the RFC. This way the client can handle it in a proper way.

See http://tools.ietf.org/html/rfc6455#section-4.2.2

Member

normanmaurer commented Jan 23, 2014

The problem is that at the moment we throw an WebSocketHandshakeException on the server-side. The proper way would be to just not send back the sub-protocol header like stated in the RFC. This way the client can handle it in a proper way.

See http://tools.ietf.org/html/rfc6455#section-4.2.2

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer
Member

normanmaurer commented Jan 23, 2014

@trustin wdyt ?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 23, 2014

Build result for #2149 at b8c486d: Success

ghost commented Jan 23, 2014

Build result for #2149 at b8c486d: Success

@@ -136,7 +136,9 @@ protected FullHttpResponse newHandshakeResponse(FullHttpRequest req, HttpHeaders
if (subprotocols != null) {
String selectedSubprotocol = selectSubprotocol(subprotocols);
if (selectedSubprotocol == null) {
throw new WebSocketHandshakeException("Requested subprotocol(s) not supported: " + subprotocols);
if (logger.isDebugEnabled()) {
logger.debug(String.format("Requested subprotocol(s) not supported: %s.", subprotocols));

This comment has been minimized.

@trustin

trustin Jan 24, 2014

Member

logger.debug("Requested subprotocol(s) not supported: {}", subprotocols); should work. Same for the other debug messages.

@trustin

trustin Jan 24, 2014

Member

logger.debug("Requested subprotocol(s) not supported: {}", subprotocols); should work. Same for the other debug messages.

@trustin

This comment has been minimized.

Show comment
Hide comment
@trustin

trustin Jan 24, 2014

Member

If no subprotocol was accepted, it sounds like the upgrade request should not be accepted (i.e. the server should not respond with '101 Switching Protocol'). What would be the best status code for this? /cc @veebs

Member

trustin commented Jan 24, 2014

If no subprotocol was accepted, it sounds like the upgrade request should not be accepted (i.e. the server should not respond with '101 Switching Protocol'). What would be the best status code for this? /cc @veebs

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 24, 2014

Member

@trustin Actually I think it should response with 101 Switching Protocol from the RFC. Why you think it should not ?

Member

normanmaurer commented Jan 24, 2014

@trustin Actually I think it should response with 101 Switching Protocol from the RFC. Why you think it should not ?

@trustin

This comment has been minimized.

Show comment
Hide comment
@trustin

trustin Jan 24, 2014

Member

I see. Instead we should not modify the pipeline, but just close the connection after sending the response then. Because the client MUST fail the connection, there's no point of keeping the connection in my opinion. Also does our client handle this situation? Can you please add a test case?

Member

trustin commented Jan 24, 2014

I see. Instead we should not modify the pipeline, but just close the connection after sending the response then. Because the client MUST fail the connection, there's no point of keeping the connection in my opinion. Also does our client handle this situation? Can you please add a test case?

@trustin

This comment has been minimized.

Show comment
Hide comment
@trustin

trustin Jan 24, 2014

Member

i.e. When there's no subprotocol available, our client handshaker must raise an exception and close the connection automatically.

Member

trustin commented Jan 24, 2014

i.e. When there's no subprotocol available, our client handshaker must raise an exception and close the connection automatically.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 24, 2014

Member

@trustin Why the client MUST fail the connection ? I was not able to spot this in the RFC. Can you point me to it ?

Member

normanmaurer commented Jan 24, 2014

@trustin Why the client MUST fail the connection ? I was not able to spot this in the RFC. Can you point me to it ?

@trustin

This comment has been minimized.

Show comment
Hide comment
@trustin

trustin Jan 24, 2014

Member

sorry I misread this:

  1. If the response includes a |Sec-WebSocket-Protocol| header field
    and this header field indicates the use of a subprotocol that was
    not present in the client's handshake (the server has indicated a
    subprotocol not requested by the client), the client MUST Fail
    the WebSocket Connection
    .

If the server's response does not conform to the requirements for the
server's handshake as defined in this section and in Section 4.2.2,
the client MUST Fail the WebSocket Connection.

But why does the RFC not mention the case where the server has no accepted subprotocols? What would be the most sensible behavior that does not violate the RFC?

Sent from a mobile device.
https://twitter.com/trustin
https://twitter.com/trustin_ko
https://twitter.com/netty_project

-----Original Message-----
From: Norman Maurer notifications@github.com
To: netty/netty netty@noreply.github.com
Cc: Trustin Lee t@motd.kr
Sent: 금, 24 1월 2014 5:12 오후
Subject: Re: [netty] Not throw an exception if subprotocol is not supported but just drop the... (#2149)

@trustin Why the client MUST fail the connection ? I was not able to spot this in the RFC. Can you point me to it ?


Reply to this email directly or view it on GitHub:
#2149 (comment)

Member

trustin commented Jan 24, 2014

sorry I misread this:

  1. If the response includes a |Sec-WebSocket-Protocol| header field
    and this header field indicates the use of a subprotocol that was
    not present in the client's handshake (the server has indicated a
    subprotocol not requested by the client), the client MUST Fail
    the WebSocket Connection
    .

If the server's response does not conform to the requirements for the
server's handshake as defined in this section and in Section 4.2.2,
the client MUST Fail the WebSocket Connection.

But why does the RFC not mention the case where the server has no accepted subprotocols? What would be the most sensible behavior that does not violate the RFC?

Sent from a mobile device.
https://twitter.com/trustin
https://twitter.com/trustin_ko
https://twitter.com/netty_project

-----Original Message-----
From: Norman Maurer notifications@github.com
To: netty/netty netty@noreply.github.com
Cc: Trustin Lee t@motd.kr
Sent: 금, 24 1월 2014 5:12 오후
Subject: Re: [netty] Not throw an exception if subprotocol is not supported but just drop the... (#2149)

@trustin Why the client MUST fail the connection ? I was not able to spot this in the RFC. Can you point me to it ?


Reply to this email directly or view it on GitHub:
#2149 (comment)

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 24, 2014

Member

@trustin I think we should just send back the upgrade response and let the client decide what to do. Sounds like the safest solution.

Member

normanmaurer commented Jan 24, 2014

@trustin I think we should just send back the upgrade response and let the client decide what to do. Sounds like the safest solution.

@trustin

This comment has been minimized.

Show comment
Hide comment
@trustin
Member

trustin commented Jan 24, 2014

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 24, 2014

Member

@trustin so ok to merge in after addressing your comments...

Member

normanmaurer commented Jan 24, 2014

@trustin so ok to merge in after addressing your comments...

@trustin

This comment has been minimized.

Show comment
Hide comment
@trustin

trustin Jan 24, 2014

Member

Yeah. It would be great if you still add a test case for this though.

Member

trustin commented Jan 24, 2014

Yeah. It would be great if you still add a test case for this though.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 24, 2014

Member

will do..

-- 
Norman Maurer

An 24. Januar 2014 at 13:02:37, Trustin Lee (notifications@github.com) schrieb:

Yeah. It would be great if you still add a test case for this though.


Reply to this email directly or view it on GitHub.

Member

normanmaurer commented Jan 24, 2014

will do..

-- 
Norman Maurer

An 24. Januar 2014 at 13:02:37, Trustin Lee (notifications@github.com) schrieb:

Yeah. It would be great if you still add a test case for this though.


Reply to this email directly or view it on GitHub.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 26, 2014

Build result for #2149 at ef9eef9: Success

ghost commented Jan 26, 2014

Build result for #2149 at ef9eef9: Success

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 26, 2014

Member

cherry-picked with tests :)

Member

normanmaurer commented Jan 26, 2014

cherry-picked with tests :)

@normanmaurer normanmaurer deleted the ws_subprotocol branch Feb 8, 2014

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