Problem with sending large responses

[molnarg]

As reported by @shigeki in #15:

When I GET a large.html file which is more than 10Mbyte. The error is

unixjp:~/tmp/github/node-http2/example>       ~/tmp/oldnode/node-v0.10.15/node server.js
Listening on localhost:8443, serving up files from /home/ohtsu/tmp/github/node-http2/example
Incoming request: /large.html (/home/ohtsu/tmp/github/node-http2/example/large.html)
Reading file from disk.

/home/ohtsu/tmp/github/node-http2/lib/stream.js:350
      throw new Error('Sending illegal frame (' + frame.type + ') in ' + this.
            ^
Error: Sending illegal frame (DATA) in CLOSED state.
    at Stream.transition [as _transition] (/home/ohtsu/tmp/github/node-http2/lib/stream.js:350:13)
    at Duplex.EventEmitter.emit (events.js:95:17)
    at Stream._flush (/home/ohtsu/tmp/github/node-http2/lib/stream.js:122:19)
    at processImmediate [as _immediateCallback] (timers.js:330:15)

[molnarg]

Could you try it with logging enabled and with pulling today's updates from git? I've just committed a few improvement to flow control. The appropriate command would be:

HTTP2_LOG=debug ~/tmp/oldnode/node-v0.10.15/node server.js

(it should put out lots of JSON based log messages, see logging section in README)

[tatsuhiro-t]

This is probably the handling of incoming WINDOW_UPDATE in half-closed (remote) state.
In stream.js#L301, WINDOW_UPDATE in that state is handled as PROTOCOL_ERROR and stream becomes
CLOSED state.
And at the end of the function, DATA is sent to the closed stream and gets this error.
WINDOW_UPDATE should be accepted in half-closed (remote) because usually client sends GET method without request body, and when receiving response, it is half-closed (remote) from the server's point of view.

[molnarg]

Yeah, you're right, that's probably an incoming HEADERS+END_STREAM followed by an incoming WINDOW_UPDATE followed by an outgoing DATA.

The spec says:

half closed (remote): 
      A stream that is "half closed (remote)" is no longer being used by
      the peer to send frames.  In this state, an endpoint is no longer
      obligated to maintain a receiver flow control window if it
      performs flow control.

      If an endpoint receives additional frames for a stream that is in
      this state it MUST respond with a stream error (Section 5.4.2) of
      type STREAM_CLOSED.

So the WINDOW_UDPATE should not be accepted, but flow control should be disabled when the stream gets into "half-closed (remote)" state. Another work item is to handle this with STREAM_CLOSED error code.

[molnarg]

Or did I misunderstand the text? For the second reading, it says that we don't have to do flow control in the other direction, since the peer won't send more data anyway...

[mcmanus]

see also httpwg/http2-spec#206

the spec basically needed to be loosened. WINDOW_UPDATE after even full close is always going to be possible unless you require a RTT based ACK of END_STREAM to transition to close (which we really don't want). So imo basically just ignore the update if you aren't sending on the stream anymore (but not if it is in half-closed-remote).

[molnarg]

Thanks for the reference and the clarification!

[molnarg]

@shigeki can you confirm this is fixed?

[shigeki]

@molnarg Sorry for my late reply. I'm back to work today. I tried to test this at the latest master and found that the issue was fixed. My test was intended to check if a flow control works fine with my client but I found today that node-http2 sends initial settings to disable the flow control together with a initial window size as below. Is it intended to implement the flow control in future?

LOG: OnSETTINGS { type: 4,
  flags: 0,
  stream_id: 0,
  payload: <Buffer 00 00 00 04 00 00 00 64 00 00 00 07 00 01 86 a0 00 00 00 0a 00 00 00 01>,
  length: 24,
  setting_block:
   { SETTINGS_MAX_CONCURRENT_STREAMS: 100,
     SETTINGS_INITIAL_WINDOW_SIZE: 100000,
     SETTINGS_FLOW_CONTROL_OPTIONS: 1 } } http_client

[molnarg]

OK, thanks for the confirmation. The reason why SETTINGS_INITIAL_WINDOW_SIZE is sent along with SETTINGS_FLOW_CONTROL_OPTIONS is that SETTINGS_INITIAL_WINDOW_SIZE is mandatory according to the spec!

I plan to implement a simple flow control algorithm in the coming days, and the progress will be tracked in #28. Until that is done, I would recommend testing with nghttp, it implements flow control properly.

[shigeki]

I tried this test of node-http2 as a SSL server with NPN. I think that SETTINGS_INITIAL_WINDOW_SIZE and SETTINGS_MAX_CONCURRENT_STREAMS are only necessary in a HTTP2-Settings header sent by a client. as described in "3.2.1. HTTP2-Settings Header Field" and they are not mandatory sent by a server with NPN. Did I miss something?

[shigeki]

I'm aware that httpwg/http2-spec#150 is in the issue lists but it has not been commited yet.

[molnarg]

You're right, I though it was necessary all the time. Will remove for now, and add back when the next draft comes out (if it makes these mandatory).