Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

connection: ignore window updates after end stream #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

connection: ignore window updates after end stream #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/spdy-transport/connection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,10 @@ Connection.prototype._handleFrame = function _handleFrame (frame) {
// Other side should destroy the stream upon receiving GOAWAY
if (this._isGoaway(frame.id)) { return }

// Ignore WINDOW_UPDATE frames from closed streams
if (frame.type === 'WINDOW_UPDATE' &&
frame.id <= state.stream.lastId.received) { return }

state.debug('id=0 stream=%d not found', frame.id)
state.framer.rstFrame({ id: frame.id, code: 'INVALID_STREAM' })
return
Expand Down
24 changes: 24 additions & 0 deletions test/both/transport/connection-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,30 @@ describe('Transport/Connection', function () {
})
})

it('should ignore WINDOW_UPDATE frame after END_STREAM', function (done) {
client.request({
path: '/hello'
}, function (err, stream) {
assert(!err)

stream.resume()

stream.once('close', function () {
client.on('frame', function (frame) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 306 should be server.on('frame', ..., since it will be sent by the client and ignored by the server.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this change the tests no longer pass.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meaning that the frame keeps being emitted? Could you c&p the error log?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

  8) Transport/Push spdy (v2) should not error on extra PRIORITY frame:

      Uncaught AssertionError: 'DATA' != 'DATA'
      + expected - actual


      at Connection.<anonymous> (C:\Users\dougw\GitHub\nodejs-spdy-transport\test\both\transport\push-test.js:339:16)
      at Connection._handleFrame (C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\connection.js:273:8)
      at Parser.<anonymous> (C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\connection.js:155:10)
      at readableAddChunk (C:\Users\dougw\GitHub\nodejs-spdy-transport\node_modules\readable-stream\lib\_stream_readable.js:217:18)
      at Parser.Readable.push (C:\Users\dougw\GitHub\nodejs-spdy-transport\node_modules\readable-stream\lib\_stream_readable.js:176:10)
      at Parser.Transform.push (C:\Users\dougw\GitHub\nodejs-spdy-transport\node_modules\readable-stream\lib\_stream_transform.js:123:32)
      at next (C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\protocol\base\parser.js:53:12)
      at C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\protocol\spdy\parser.js:57:5
      at Parser.onFrameBody (C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\protocol\spdy\parser.js:132:12)
      at Parser.execute (C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\protocol\spdy\parser.js:49:8)
      at Parser._consume (C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\protocol\base\parser.js:95:8)
      at C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\protocol\base\parser.js:62:12
      at _combinedTickCallback (internal/process/next_tick.js:73:7)
      at process._tickCallback (internal/process/next_tick.js:104:9)

  9) SPDY Transport autoSpdy31 should automatically switch on server:
     Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.
      at Timeout.<anonymous> (C:\Users\dougw\GitHub\nodejs-spdy-transport\node_modules\mocha\lib\runnable.js:226:19)

assert(!(frame.type === 'RST' && frame.id === 1))
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you could move this assert (306~308) to outside of the stream.once('close', you could send multiple windowUpdateFrame and see it being processed before and confirming that it doesn't get emitted after the close happens.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this change makes the tests no longer pass.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

  4) Transport/Connection spdy (v3.1) should ignore WINDOW_UPDATE frame after END_STREAM:

      Uncaught AssertionError: false == true
      + expected - actual

      -false
      +true

      at Connection.<anonymous> (C:\Users\dougw\GitHub\nodejs-spdy-transport\test\both\transport\connection-test.js:305:13)
      at Connection._handleFrame (C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\connection.js:273:8)
      at Parser.<anonymous> (C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\connection.js:155:10)
      at readableAddChunk (C:\Users\dougw\GitHub\nodejs-spdy-transport\node_modules\readable-stream\lib\_stream_readable.js:217:18)
      at Parser.Readable.push (C:\Users\dougw\GitHub\nodejs-spdy-transport\node_modules\readable-stream\lib\_stream_readable.js:176:10)
      at Parser.Transform.push (C:\Users\dougw\GitHub\nodejs-spdy-transport\node_modules\readable-stream\lib\_stream_transform.js:123:32)
      at next (C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\protocol\base\parser.js:53:12)
      at C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\protocol\spdy\parser.js:57:5
      at Parser.onRSTFrame (C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\protocol\spdy\parser.js:387:3)
      at Parser.onFrameBody (C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\protocol\spdy\parser.js:143:10)
      at Parser.execute (C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\protocol\spdy\parser.js:49:8)
      at Parser._consume (C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\protocol\base\parser.js:95:8)
      at C:\Users\dougw\GitHub\nodejs-spdy-transport\lib\spdy-transport\protocol\base\parser.js:62:12
      at _combinedTickCallback (internal/process/next_tick.js:73:7)
      at process._tickCallback (internal/process/next_tick.js:104:9)


client._spdyState.framer.windowUpdateFrame({
id: stream.id,
delta: 1
}, function (err) {
assert(!err)
setTimeout(done, 100)
})
})
})
})

it('should use last received id when killing streams', function (done) {
var waiting = 2
function next () {
Expand Down