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 <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
  • Loading branch information
oyyd authored and MylesBorins committed Oct 30, 2018
1 parent 2564abf commit 1afd207
Show file tree
Hide file tree
Showing 2 changed files with 51 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
48 changes: 48 additions & 0 deletions test/parallel/test-stream-wrap-drain.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const { StreamWrap } = require('_stream_wrap');
const { Duplex } = require('stream');
const { ShutdownWrap } = process.binding('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 1afd207

Please sign in to comment.