Skip to content

Commit

Permalink
tls: make StreamWrap work correctly in "drain" callback
Browse files Browse the repository at this point in the history
When an instance of StreamWrap is shutting down and a "drain" event
is emitted, the instance will abort as its
`this[kCurrentShutdownRequest]` is already set. The following test
will fail before this commit.

PR-URL: #23294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
  • Loading branch information
oyyd authored and jasnell committed Oct 17, 2018
1 parent 51fd867 commit 318f1cd
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 4 deletions.
7 changes: 3 additions & 4 deletions lib/internal/wrap_js_stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,6 @@ class JSStreamWrap extends Socket {
}

doShutdown(req) {
assert.strictEqual(this[kCurrentShutdownRequest], null);
this[kCurrentShutdownRequest] = req;

// TODO(addaleax): It might be nice if we could get into a state where
// DoShutdown() is not called on streams while a write is still pending.
//
Expand All @@ -113,8 +110,10 @@ class JSStreamWrap extends Socket {
// so for now that is supported here.

if (this[kCurrentWriteRequest] !== null)
return this.on('drain', () => this.doShutdown(req));
return this.once('drain', () => this.doShutdown(req));
assert.strictEqual(this[kCurrentWriteRequest], null);
assert.strictEqual(this[kCurrentShutdownRequest], null);
this[kCurrentShutdownRequest] = req;

const handle = this._handle;

Expand Down
50 changes: 50 additions & 0 deletions test/parallel/test-stream-wrap-drain.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Flags: --expose-internals
'use strict';
const common = require('../common');
const assert = require('assert');
const { StreamWrap } = require('_stream_wrap');
const { Duplex } = require('stream');
const { internalBinding } = require('internal/test/binding');
const { ShutdownWrap } = internalBinding('stream_wrap');

// This test makes sure that when an instance of JSStreamWrap is waiting for
// a "drain" event to `doShutdown`, the instance will work correctly when a
// "drain" event emitted.
{
let resolve = null;

class TestDuplex extends Duplex {
_write(chunk, encoding, callback) {
// We will resolve the write later.
resolve = () => {
callback();
};
}

_read() {}
}

const testDuplex = new TestDuplex();
const socket = new StreamWrap(testDuplex);

socket.write(
// Make the buffer long enough so that the `Writable` will emit "drain".
Buffer.allocUnsafe(socket.writableHighWaterMark * 2),
common.mustCall()
);

// Make sure that the 'drain' events will be emitted.
testDuplex.on('drain', common.mustCall(() => {
console.log('testDuplex drain');
}));

assert.strictEqual(typeof resolve, 'function');

const req = new ShutdownWrap();
req.oncomplete = common.mustCall();
req.handle = socket._handle;
// Should not throw.
socket._handle.shutdown(req);

resolve();
}

0 comments on commit 318f1cd

Please sign in to comment.