twitter-archive/CocoaSPDY

Unable to upload files more than 65536 bytes

petroakzhygitov opened this issue · 5 comments

Hello,

We've started discussion here #85, but there was no answer so I decided to open an issue.

There is a function:

(void)didReadWindowUpdateFrame:(SPDYWindowUpdateFrame )windowUpdateFrame frameDecoder:(SPDYFrameDecoder *)frameDecoder
{
/*

SPDY WINDOW_UPDATE frame processing requirements: *
Receivers of a WINDOW_UPDATE that cause the window size to exceed 2^31
must send a RST_STREAM with the status code FLOW_CONTROL_ERROR. *
Sender should ignore all WINDOW_UPDATE frames associated with a stream
after sending the last frame for the stream. */
SPDYStreamId streamId = windowUpdateFrame.streamId;
SPDY_DEBUG(@"received WINDOW_UPDATE.%u (+%lu)", streamId, (unsigned long)windowUpdateFrame.deltaWindowSize);

if (streamId == kSPDYSessionStreamId) {
// Check for numerical overflow
if (_sessionSendWindowSize > INT32_MAX - windowUpdateFrame.deltaWindowSize) {
[self _closeWithStatus:SPDY_SESSION_PROTOCOL_ERROR];
return;
}

_sessionSendWindowSize += windowUpdateFrame.deltaWindowSize;
for (SPDYStream *stream in _activeStreams) {
    [self _sendData:stream];
    if (_sessionSendWindowSize == 0) break;
}

return;
}

// Ignore frames for non-existent or half-closed streams
SPDYStream *stream = _activeStreams[streamId];
if (!stream || stream.localSideClosed) {
return;
}

// Check for numerical overflow
if (stream.sendWindowSize > INT32_MAX - windowUpdateFrame.deltaWindowSize) {
[self _sendRstStream:SPDY_STREAM_FLOW_CONTROL_ERROR streamId:streamId];
return;
}

stream.sendWindowSize += windowUpdateFrame.deltaWindowSize;
[self _sendData:stream];
}

if streamId is eqal to kSPDYSessionStreamId then _sessionSendWindowSize should be increased, but if not, stream.sendWindowSize should be increased instead. So only one sendWindowSize will be increased, not the both.

In _sendData:(SPDYStream *)stream function both of sendWindowSize were decreased at the same time:

_sessionSendWindowSize -= bytesSent;
stream.sendWindowSize -= bytesSent;

and in case of bytesSent equals 65536 bytes, they both equal 0.

Upon next call to the _sendData this line will always return 0:

uint32_t sendWindowSize = MIN(_sessionSendWindowSize, stream.sendWindowSize);

because only one sendWindowSize was increased in didReadWindowUpdateFrame.

So, a file with size more than 65536 bytes will never send.

Thanks for the detailed explanation, and sorry I didn't get back to your pull request. The receiver is expected to send 2 WINDOW_UPDATE frames. For both cases, in the didReadWindowUpdateFrame method, the appropriate window is adjusted then the _sendData method is called for all relevant streams. The SPDY 3.1 spec, under WINDOW_UPDATE section, says:

After sending a flow controlled frame, the sender reduces the space available in both windows by the length of the transmitted frame.
The receiver of a frame sends a WINDOW_UPDATE frame as it consumes data and frees up space in flow control windows. Separate WINDOW_UPDATE frames are sent for the stream and connection level flow control windows.

See http://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3-1#TOC-2.6.8-WINDOW_UPDATE.

I'm curious, what server are you using that isn't sending WINDOW_UPDATE frames for both the session and stream flow control windows?

hi @kgoodier. We are using jetty spdy server 9.2.6.
Thanks for pointing out into spec. I'll play with server side to make it work and see if it is 9.2 version bug

@chexov Did you find anything interesting?

@kgoodier thanks for waiting. I did dig it today a little bit and spdy server receives windowUpdates for the opened session.

I am wrong somewhere for sure here but bear with me. Here is debugging session of me as non iOS developer:

  1. got jetty-spdy server running and make sure that have breakpoints set on settings+windowUpdate event to catch what did client sends me.
  2. Got breakpoint triggered and cocoaspdy sends me settings with windowSize=10485760.
    It starts from
    [[SPDYSession alloc] initWithOrigin...]
    which makes it to
    SPDYSession.m:165 [self _sendWindowUpdate:deltaWindowSize streamId:kSPDYSessionStreamId]
    which calculates deltaWindowSize from _sessionReceiveWindowSize var.
  3. At this point we have cocoaspdy with _sessionSendWindowSize=65536 and spdy-server with windowSize=10485760.. which looks like a bug to me...No?
  4. I changed [SPDYSession _sendWindowUpdate] method to look like this (added one line
    _sessionSendWindowSize = deltaWindowSize; and It works for my usecase...
    Could be dirty hack but I am not familiar with the lib so close and hoping this will give you enough info to explain what I am doing wrong...

Full implementation looks like this:

- (void)_sendWindowUpdate:(uint32_t)deltaWindowSize streamId:(SPDYStreamId)streamId
{
    SPDYWindowUpdateFrame *windowUpdateFrame = [[SPDYWindowUpdateFrame alloc] init];
    windowUpdateFrame.streamId = streamId;
    windowUpdateFrame.deltaWindowSize = deltaWindowSize;
    _sessionSendWindowSize =  deltaWindowSize; // ADDED by @chexov
    [_frameEncoder encodeWindowUpdateFrame:windowUpdateFrame];
    SPDY_DEBUG(@"sent WINDOW_UPDATE.%u (+%lu)", streamId, (unsigned long)deltaWindowSize);
}

P.S. If you see no error on cocoaspdy side please let me know so I can continue to dig... really interested at fixing this. thanks!

server set window size is 2^31-1, same question appear。