Post Flow Control Clean-up

‹prev | My Chain | next›

I have SPDY flow control from version 3 of the protocol working in node-spdy. Tonight, I aim to make sure that I have not introduced unnecessary complexity to node-spdy in my implementation.

Flow control in SPDY is per-stream, so I added the internal data transfer window calculation and backlog to the Stream constructor:

function Stream(connection, frame) {
  // ...
  // Lock data
  this.locked = false;
  this.lockBuffer = [];

  // ...
  this._paused = false;
  this._buffer = [];

  this.internalWindowSize = Math.pow(2,16);
  this._backlog = [];

  // ...
}

There is a lot going in the Stream constructor, but the properties that concerned me last night were the lockBuffer and the _buffer—both sound similar to the _backlog that I added.

The lockBuffer turns out to be a buffer of callbacks, not bytes, so that won't help me. The _paused / _buffer properties, on the other hand, may prove useful. The associated resume() method even does similar work of draining the buffer after some event triggers the resumption of the stream:

Stream.prototype.resume = function resume() {
  if (!this._paused) return;
  this._paused = false;

  var self = this,
      buffer = this._buffer;

  this._buffer = [];

  process.nextTick(function() {
    buffer.forEach(function(chunk) {
      self._read(chunk);
    });
  });
};

Ah, but wait, that is a read-operation. Dang it, I ran into this yesterday as well. Node.js allows it readable streams to pause, but not the writable streams. The _paused and _buffer properties are simply supporting node-spdy's attempt to expose a complete Stream interface.

So it looks as though I need a separate transfer window buffer after all. I do think that a rename is order, however. The name backlog made a tiny bit of sense when I was mucking with global variables, but now I think a rename is in order:

// ...
  this._transferWindowSize = Math.pow(2,16);
  this._transferWindowBuffer = [];
// ...

That should make life easier.

Before calling it a night, I double check that my connections are being closed. When upstream signals the end() of a connection, I had to conditionally close the stream. It turns out the _write() method already has a built-in call to close()—separate from the call in end()—when a DATA FIN is seen:

Stream.prototype._writeData = function _writeData(fin, buffer) {
    // guard against data transfer window exhaustion...

    if (fin) this.close();
    // ...
};

A quick check verifies that the stream really is being closed properly. The following console.log() in close():

Stream.prototype.close = function close() {
  console.log(this.id + " is closed");
  this.destroy();
};

Results in all of the streams getting closed:

➜  express-spdy-test  node app
Express server listening on port 3000 in development mode
1 is closed
1 is closed
3 is closed
3 is closed
5 is closed
5 is closed
7 is closed
9 is closed
11 is closed
11 is closed

I am not fussed over the duplicate closes for the non-flow controlled (non-buffered) output streams. Closing a second time has no effect. As long as no resources remain open and allocated, everything should be fine.

Day #376