From e4fdf7149b11645504aa88c5ec43b00da161d533 Mon Sep 17 00:00:00 2001 From: Andrew Naylor Date: Sun, 1 May 2016 23:21:51 +0100 Subject: [PATCH 1/7] Cleanup ended streams to free memory Fixes #64 --- lib/protocol/connection.js | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/lib/protocol/connection.js b/lib/protocol/connection.js index 2b86b7f..72a597e 100644 --- a/lib/protocol/connection.js +++ b/lib/protocol/connection.js @@ -214,6 +214,11 @@ Connection.prototype._insert = function _insert(stream, priority) { }; Connection.prototype._reprioritize = function _reprioritize(stream, priority) { + this._removePrioritisedStream(stream); + this._insert(stream, priority); +}; + +Connection.prototype._removePrioritisedStream = function _removePrioritisedStream(stream) { var bucket = this._streamPriorities[stream._priority]; var index = bucket.indexOf(stream); assert(index !== -1); @@ -221,9 +226,7 @@ Connection.prototype._reprioritize = function _reprioritize(stream, priority) { if (bucket.length === 0) { delete this._streamPriorities[stream._priority]; } - - this._insert(stream, priority); -}; +} // Creating an *inbound* stream with the given ID. It is called when there's an incoming frame to // a previously nonexistent stream. @@ -235,6 +238,8 @@ Connection.prototype._createIncomingStream = function _createIncomingStream(id) this._allocatePriority(stream); this.emit('stream', stream, id); + stream.on('end', this._removeStream.bind(this, stream)); + return stream; }; @@ -246,9 +251,18 @@ Connection.prototype.createStream = function createStream() { var stream = new Stream(this._log, this); this._allocatePriority(stream); + stream.on('end', this._removeStream.bind(this, stream)); + return stream; }; +Connection.prototype._removeStream = function _removeStream(stream) { + this._log.trace('Removing outbound stream.'); + + delete this._streamIds[stream.id]; + this._removePrioritisedStream(stream); +} + // Multiplexing // ------------ From a7f1e30359fb265d5e5b45c10fce4847ff373e64 Mon Sep 17 00:00:00 2001 From: Andrew Naylor Date: Sun, 1 May 2016 23:48:49 +0100 Subject: [PATCH 2/7] Handle server streams correctly --- lib/protocol/connection.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/protocol/connection.js b/lib/protocol/connection.js index 72a597e..240b00e 100644 --- a/lib/protocol/connection.js +++ b/lib/protocol/connection.js @@ -238,7 +238,6 @@ Connection.prototype._createIncomingStream = function _createIncomingStream(id) this._allocatePriority(stream); this.emit('stream', stream, id); - stream.on('end', this._removeStream.bind(this, stream)); return stream; }; @@ -304,7 +303,7 @@ priority_loop: // 2. if there's no frame, skip this stream // 3. if forwarding this frame would make `streamCount` greater than `streamLimit`, skip // this stream - // 4. adding stream to the bucket of the next round + // 4. adding stream to the bucket of the next round unless it has ended // 5. assigning an ID to the frame (allocating an ID to the stream if there isn't already) // 6. if forwarding a PUSH_PROMISE, allocate ID to the promised stream // 7. forwarding the frame, changing `streamCount` as appropriate @@ -322,7 +321,11 @@ priority_loop: continue; } - nextBucket.push(stream); + if (!stream._ended) { + nextBucket.push(stream); + } else { + delete this._streamIds[stream.id]; + } if (frame.stream === undefined) { frame.stream = stream.id || this._allocateId(stream); From 9e362361a5d460452c6acc03028258441d686f16 Mon Sep 17 00:00:00 2001 From: Andrew Naylor Date: Sun, 1 May 2016 23:53:21 +0100 Subject: [PATCH 3/7] Fix style issues --- lib/protocol/connection.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/protocol/connection.js b/lib/protocol/connection.js index 240b00e..8aa313d 100644 --- a/lib/protocol/connection.js +++ b/lib/protocol/connection.js @@ -226,7 +226,7 @@ Connection.prototype._removePrioritisedStream = function _removePrioritisedStrea if (bucket.length === 0) { delete this._streamPriorities[stream._priority]; } -} +}; // Creating an *inbound* stream with the given ID. It is called when there's an incoming frame to // a previously nonexistent stream. @@ -238,7 +238,6 @@ Connection.prototype._createIncomingStream = function _createIncomingStream(id) this._allocatePriority(stream); this.emit('stream', stream, id); - return stream; }; @@ -260,7 +259,7 @@ Connection.prototype._removeStream = function _removeStream(stream) { delete this._streamIds[stream.id]; this._removePrioritisedStream(stream); -} +}; // Multiplexing // ------------ From 9e81bd5c9b776e77d7b63f2a8369634d202459b5 Mon Sep 17 00:00:00 2001 From: Simon Tretter Date: Thu, 20 Oct 2016 02:09:38 +0200 Subject: [PATCH 4/7] Update connection.js --- lib/protocol/connection.js | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/lib/protocol/connection.js b/lib/protocol/connection.js index 2b86b7f..8aa313d 100644 --- a/lib/protocol/connection.js +++ b/lib/protocol/connection.js @@ -214,6 +214,11 @@ Connection.prototype._insert = function _insert(stream, priority) { }; Connection.prototype._reprioritize = function _reprioritize(stream, priority) { + this._removePrioritisedStream(stream); + this._insert(stream, priority); +}; + +Connection.prototype._removePrioritisedStream = function _removePrioritisedStream(stream) { var bucket = this._streamPriorities[stream._priority]; var index = bucket.indexOf(stream); assert(index !== -1); @@ -221,8 +226,6 @@ Connection.prototype._reprioritize = function _reprioritize(stream, priority) { if (bucket.length === 0) { delete this._streamPriorities[stream._priority]; } - - this._insert(stream, priority); }; // Creating an *inbound* stream with the given ID. It is called when there's an incoming frame to @@ -246,9 +249,18 @@ Connection.prototype.createStream = function createStream() { var stream = new Stream(this._log, this); this._allocatePriority(stream); + stream.on('end', this._removeStream.bind(this, stream)); + return stream; }; +Connection.prototype._removeStream = function _removeStream(stream) { + this._log.trace('Removing outbound stream.'); + + delete this._streamIds[stream.id]; + this._removePrioritisedStream(stream); +}; + // Multiplexing // ------------ @@ -290,7 +302,7 @@ priority_loop: // 2. if there's no frame, skip this stream // 3. if forwarding this frame would make `streamCount` greater than `streamLimit`, skip // this stream - // 4. adding stream to the bucket of the next round + // 4. adding stream to the bucket of the next round unless it has ended // 5. assigning an ID to the frame (allocating an ID to the stream if there isn't already) // 6. if forwarding a PUSH_PROMISE, allocate ID to the promised stream // 7. forwarding the frame, changing `streamCount` as appropriate @@ -308,7 +320,11 @@ priority_loop: continue; } - nextBucket.push(stream); + if (!stream._ended) { + nextBucket.push(stream); + } else { + delete this._streamIds[stream.id]; + } if (frame.stream === undefined) { frame.stream = stream.id || this._allocateId(stream); From 542166028c400fef935004e07ffc4924763383b9 Mon Sep 17 00:00:00 2001 From: Simon Tretter Date: Thu, 20 Oct 2016 09:38:51 +0200 Subject: [PATCH 5/7] Update connection.js --- lib/protocol/connection.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/protocol/connection.js b/lib/protocol/connection.js index 8aa313d..75012e9 100644 --- a/lib/protocol/connection.js +++ b/lib/protocol/connection.js @@ -311,6 +311,7 @@ priority_loop: while (bucket.length > 0) { for (var index = 0; index < bucket.length; index++) { var stream = bucket[index]; + if(!stream || !stream.upstream) continue; var frame = stream.upstream.read((this._window > 0) ? this._window : -1); if (!frame) { From 074a654024a508b7f7110673c79a7fa72a894d2c Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Tue, 7 Mar 2017 11:13:09 -0800 Subject: [PATCH 6/7] Remove invalid assert Fixes #228 In the case where this._push(frame) returns null (i.e., the frame is too large for the window and split or the window size is <=0), moreNeeded will be set to null. Then this._queue.push(frame) is called, but moreNeeded is still null. Thus, any time the window is <=0 or the frame is split we'll hit the assert: var moreNeeded = null; if (this._queue.length === 0) { moreNeeded = this._push(frame); } if (moreNeeded === null) { this._queue.push(frame); } return moreNeeded; Credit goes to @jrabek for original version of this patch --- lib/protocol/connection.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/protocol/connection.js b/lib/protocol/connection.js index 75012e9..a552d2c 100644 --- a/lib/protocol/connection.js +++ b/lib/protocol/connection.js @@ -340,8 +340,7 @@ priority_loop: var moreNeeded = this.push(frame); this._changeStreamCount(frame.count_change); - assert(moreNeeded !== null); // The frame shouldn't be unforwarded - if (moreNeeded === false) { + if (!moreNeeded) { break priority_loop; } } From dff41d97f010c9edb6369f02b382810a4f0bdbf3 Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Tue, 7 Mar 2017 10:48:20 -0800 Subject: [PATCH 7/7] Fix GOAWAY deserialization when debug data is present Additional debug data is allowed to be included in the GOAWAY frame: https://http2.github.io/http2-spec/#GOAWAY. We now put that data into frame.debug_data instead of returning a FRAME_SIZE_ERROR. Fixes #218 and #219. --- lib/protocol/framer.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/protocol/framer.js b/lib/protocol/framer.js index 244e60a..4e273df 100644 --- a/lib/protocol/framer.js +++ b/lib/protocol/framer.js @@ -736,6 +736,8 @@ typeSpecificAttributes.GOAWAY = ['last_stream', 'error']; // +-+-------------------------------------------------------------+ // | Error Code (32) | // +---------------------------------------------------------------+ +// | Additional Debug Data (*) | +// +---------------------------------------------------------------+ // // The last stream identifier in the GOAWAY frame contains the highest numbered stream identifier // for which the sender of the GOAWAY frame has received frames on and might have taken some action @@ -759,8 +761,8 @@ Serializer.GOAWAY = function writeGoaway(frame, buffers) { }; Deserializer.GOAWAY = function readGoaway(buffer, frame) { - if (buffer.length !== 8) { - // GOAWAY must have 8 bytes + if (buffer.length < 8) { + // GOAWAY must have at least 8 bytes return 'FRAME_SIZE_ERROR'; } frame.last_stream = buffer.readUInt32BE(0) & 0x7fffffff; @@ -769,6 +771,12 @@ Deserializer.GOAWAY = function readGoaway(buffer, frame) { // Unknown error types are to be considered equivalent to INTERNAL ERROR frame.error = 'INTERNAL_ERROR'; } + // Read remaining data into "debug_data" + // https://http2.github.io/http2-spec/#GOAWAY + // Endpoints MAY append opaque data to the payload of any GOAWAY frame + if (buffer.length > 8) { + frame.debug_data = buffer.slice(8); + } }; // [WINDOW_UPDATE](https://tools.ietf.org/html/rfc7540#section-6.9)