Fix auto-upgrade from SPDY/3 to SPDY/3.1 #31
Conversation
| @@ -281,7 +281,7 @@ Connection.prototype._handleFrame = function _handleFrame(frame) { | |||
|
|
|||
| // Session window update | |||
| if (frame.type === 'WINDOW_UPDATE' && frame.id === 0) { | |||
| if (state.version < 3.1 && state.autoSpdy31) { | |||
There was a problem hiding this comment.
I believe the logic here was: if we receive id=0 WINDOW_UPDATE and we are on a version 3 (or less, yeah it should be === 3 here) and we want to auto-upgrade - do it.
Why do you suggest that autoSpdy31 should be removed from this if?
There was a problem hiding this comment.
The case I'm working with is in plain mode.
If autoSpdy31 is set to false, then the only way for the SPDY server to detect a SPDY/3.1 client is the id=0 WINDOW_UPDATE. However, since autoSpdy31 is set to false, the highlighted line above causes the SPDY/3.1 client to be treated as a SPDY/3 client, leading to issues when the amount of data transferred exceeds the INITIAL_WINDOW_SIZE for the connection. With no session-level flow control in SPDY/3, the SPDY/3.1 client stops transmitting after the INITIAL_WINDOW_SIZE bytes have been sent and no recovery is possible because SPDY/3 doesn't deal with id=0 WINDOW_UPDATE frames.
On the other hand, when we set autoSpdy31 to true, the SPDY server sends an id=0 WINDOW_UPDATE (https://github.com/indutny/spdy-transport/blob/master/lib/spdy-transport/connection.js#L246-L247) when a new client connects (due to the second half of the OR expression). This works fine for SPDY/3.1 clients, but with a SPDY/3 client, the id=0 WINDOW_UPDATE results in a GOAWAY frame from the client since SPDY/3 doesn't deal with session-level flow control (iSPDY does this because it fails to locate a stream with id=0 and decides to drop the connection: https://github.com/Voxer/iSPDY/blob/master/src/ispdy.m#L1227-L1242).
Thus, it seems that the autoSpdy31 flag serves as a big hammer that treats all clients as SPDY/3.1 clients when the flag is set, and as SPDY/3 clients when the flag is unset.
What we're looking for is the ability to handle both SPDY/3 and SPDY/3.1 clients in plain mode.
Removing the autoSpdy31 flag check in the line highlighted above produces the desired result, gracefully upgrading SPDY/3.1 clients, while allowing SPDY/3 clients to proceed unmodified.
There was a problem hiding this comment.
@indutny Does the above explanation work?
There was a problem hiding this comment.
Sorry for delay! Not really.
If autoSpdy31 is false - the update from 3 to 3.1 should not happen at all. That's why we have this option in place. Actually, this option makes sense only with plain: true, because otherwise protocol version is negotiated via NPN.
There was a problem hiding this comment.
Is there a way to get both SPDY/3 and SPDY/3.1 clients to work in plain mode? In its current form, the autoSpdy31 flag causes the library to be purely binary with respect to v3 and v3.1. Its all or nothing.
There was a problem hiding this comment.
So autoSpdy31: true, plain: true doesn't work now? What happens?
There was a problem hiding this comment.
So in the current case, with autoSpdy31: true, plain: true, SPDY/3.1 clients work fine. But with a SPDY/3 client, the server sends a id=0 WINDOW_UPDATE because of https://github.com/indutny/spdy-transport/blob/master/lib/spdy-transport/connection.js#L246-L247 and https://github.com/indutny/spdy-transport/blob/master/lib/spdy-transport/connection.js#L475. Because SPDY/3 doesn't implement session-level flow-control, a SPDY/3 client may treat that frame as a protocol error/violation and drop the connection (as iSPDY does because it fails to locate a stream with id=0 and decides to drop the connection: https://github.com/Voxer/iSPDY/blob/master/src/ispdy.m#L1227-L1242).
I've pushed a fix for this issue, by adding && state.version >= 3.1 to the checks. This ensures that the id=0 WINDOW_UPDATE is only sent for SPDY/3.1 clients when autoSpdy31 is set to true.
Without this additional check, the server sends id=0 WINDOW_UPDATE to a SPDY/3 client causing it to disconnect.
There was a problem hiding this comment.
Just to be clear, I made additional changes that partially revert my previous change (I put the && state.autoSpdy31 check back in) and added the && state.version >= 3.1 condition to https://github.com/indutny/spdy-transport/blob/master/lib/spdy-transport/connection.js#L246 and https://github.com/indutny/spdy-transport/blob/master/lib/spdy-transport/connection.js#L475.
So now, the server doesn't send a id=0 WINDOW_UPDATE as soon as the client connects. Instead it waits for the client to send up a id=0 WINDOW_UPDATE, upgrades the connection to SPDY/3.1 and then sends the id=0 WINDOW_UPDATE.
There was a problem hiding this comment.
Thank you! This sounds very reasonable to me!
I'll review it tomorrow.
The current code auto-upgrades all SPDY/3 clients to SPDY/3.1 when operating in **plain** mode with **autoSpdy31** set to true. This seems rather restrictive, implying that with autoSpdy31 set to true, the server does not support SPDY/3-only clients. This commit fixes this by only upgrading SPDY/3 clients to SPDY/3.1 when the server receives a WINDOW_UPDATE for stream id 0. This ensures that SPDY/3 clients can proceed uninhibited when operating in **plain** mode, and ensuring that the autoSpdy31 option is only used for SPDY/3.1 clients in **plain** mode, allowing for both SPDY/3 and SPDY/3.1 clients to co-exist on the same server in **plain** mode.
anandsuresh
force-pushed
the
fix/autoSpdy31
branch
from
b44fdb7
to
fbecced
Compare
|
@indutny Are there any more changes needed on this one? If not, can this be merged and released? Thanks. |
|
@indutny Can we merge this if you're satisfied with the review? |
The current code auto-upgrades all
spdy/3clients tospdy/3.1whenoperating in
plainmode withautoSpdy31set totrue. This seemsrather restrictive, implying that the server does not support
SPDY/3-onlyclients with
autoSpdy31set totrue, .This commit fixes this by only upgrading
spdy/3clients tospdy/3.1whenthe server receives a
WINDOW_UPDATEfor stream id 0. This ensuresthat
spdy/3clients can proceed uninhibited when operating inplainmode, and ensuring that the
autoSpdy31option is only used forspdy/3.1 clients in
plainmode, allowing for both spdy/3 andspdy/3.1 clients to co-exist on the same server.