Flow control may block non-flow-controlled frames

[nwgh]

For all frame types, lib/stream.js uses this._pushUpstream to submit the frame for sending, which in turn calls Flow.prototype.push. In Flow.prototype.push, we could conceivably get into a state where there are DATA frames queued (so this._queue.length !== 0), so moreNeeded will not get set to non-null, and the check on line 278 will evaluate to true, thus enqueuing the frame, regardless of what type of frame it is. We should check the frame type in Flow.prototype.push before checking moreNeeded at all, so we don't end up blocking non-flow-controlled frames (I think).

I can whip up a patch for this, but I wanted to submit the issue to make sure I'm not crazy before doing the work :)

[molnarg]

Hm, what we lose with this is the guarantee that frames are sent (and received) in the order they are written in the stream. I looked this up in the spec and it says:

The order in which frames are sent on a stream is significant. Recipients process frames in the order they are received. In particular, the order of HEADERS, and DATA frames is semantically significant.

So I don't think we can optimize this.

[nwgh]

I'm not so sure about that. It may not be as simple as I was originally thinking, but I think it's doable. When I was looking at this, I wasn't thinking about HEADERS or DATA particularly, this came up when I was implementing ALTSVC. Here are some things that we would need to guard against:

• Sending any DATA before we've sent the full header block (HEADERS|PUSH_PROMISE CONTINUATION*) (because duh)
• Reordering frames within a header block or a sequence of DATA frames (because duh)
• Sending a SETTINGS that updates the compression context out-of-order with a header block (since the header block would depend on the previous max header table size)
• Sending a RST_STREAM if we have any data waiting (since it's possible that the other endpoint may want to do something with the data that we've sent, such as saving a partial download for a later range request)

The only other one that could get a little weird is sending a PRIORITY, though if we're sending DATA on an particular stream, we're probably not the one that should be sending PRIORITY frames for that stream, anyway. (Obviously we wouldn't want to send PRIORITY before HEADERS.)

I don't see any issues sending, for example, ALTSVC on a stream that's blocked on flow control. I can even see a reason to actually do so - if you believe the client isn't going to be opening its window any time soon but that it's not malicious (not an incredibly likely scenario, but still...), you may want to try to redirect it to get a new connection with a new flow control window.

[molnarg]

Yeah, it's doable in certain scenarios. I'm OK with handling the cases we know are safe specially, but I'm afraid of using very complicated logic to do this. How about implementing a _pushUpstreamWithoutFlowControl() method on the Stream class as an alternative to _pushUpstream() which could be used in cases when we know this can not cause trouble (like ALTSVC)?

It would look like this:

Stream.prototype._pushUpstreamWithoutFlowControl = function _pushUpstreamWithoutFlowControl(frame) {
  this.upstream._parentPush(frame); // Logs the frame and pushed directly into the output queue
  this._transition(true, frame);
};

[nwgh]

Yeah, I think that seems like a reasonable way to go about it. Once draft14 lands (which should be as soon as Martin has the spec revision ready, unless he surprises me with some last-minute changes), I'll work on adding this.