twitter-archive / CocoaSPDY

SPDY for iOS and OS X
Apache License 2.0
2.39k stars 233 forks source link

Unable to upload files more than 65536 bytes #91

Open petroakzhygitov opened 9 years ago

petroakzhygitov commented 9 years ago

Hello,

We've started discussion here https://github.com/twitter/CocoaSPDY/pull/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.

kgoodier commented 9 years ago

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?

chexov commented 9 years ago

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

kgoodier commented 9 years ago

@chexov Did you find anything interesting?

chexov commented 9 years ago

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

firstka commented 8 years ago

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