quic: properly pass readable/writable constructor options

PR-URL: #34283 Reviewed-By: Anna Henningsen <anna@addaleax.net>
nodejs · Jul 16, 2020 · ed48822 · ed48822
1 parent 57c1129
commit ed48822
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 26 deletions.
diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js
@@ -472,21 +472,11 @@ function onStreamReady(streamHandle, id, push_id) {
   // state because new streams should not have been accepted at the C++
   // level.
   assert(!session.closing);
-
-  // TODO(@jasnell): Get default options from session
-  const uni = id & 0b10;
-  const {
-    highWaterMark,
-    defaultEncoding,
-  } = session[kStreamOptions];
   const stream = new QuicStream({
-    writable: !uni,
-    highWaterMark,
-    defaultEncoding,
+    ...session[kStreamOptions],
+    writable: !(id & 0b10),
   }, session, push_id);
   stream[kSetHandle](streamHandle);
-  if (uni)
-    stream.end();
   session[kAddStream](id, stream);
   process.nextTick(emit.bind(session, 'stream', stream));
 }
@@ -2145,12 +2135,6 @@ class QuicSession extends EventEmitter {
       readable: !halfOpen
     }, this);
 
-    // TODO(@jasnell): This really shouldn't be necessary
-    if (halfOpen) {
-      stream.push(null);
-      stream.read();
-    }
-
     state.pendingStreams.add(stream);
 
     // If early data is being used, we can create the internal QuicStream on the
@@ -2516,7 +2500,7 @@ class QuicClientSession extends QuicSession {
 }
 
 function streamOnResume() {
-  if (!this.destroyed)
+  if (!this.destroyed && this.readable)
     this[kHandle].readStart();
 }
 
@@ -2546,9 +2530,13 @@ class QuicStream extends Duplex {
     const {
       highWaterMark,
       defaultEncoding,
+      readable = true,
+      writable = true,
     } = options;
 
     super({
+      readable,
+      writable,
       highWaterMark,
       defaultEncoding,
       allowHalfOpen: true,
@@ -2996,10 +2984,6 @@ class QuicStream extends Duplex {
       defaultEncoding,
     }, this.session);
 
-    // TODO(@jasnell): The null push and subsequent read shouldn't be necessary
-    stream.push(null);
-    stream.read();
-
     stream[kSetHandle](handle);
     this.session[kAddStream](stream.id, stream);
     return stream;

diff --git a/test/parallel/test-quic-client-server.js b/test/parallel/test-quic-client-server.js
@@ -149,11 +149,13 @@ client.on('close', common.mustCall(onSocketClose.bind(client)));
       assert(uni.serverInitiated);
       assert(!uni.clientInitiated);
       assert(!uni.pending);
+      // The data and end events will never emit because
+      // the unidirectional stream is never readable.
+      uni.on('end', common.mustNotCall());
+      uni.on('data', common.mustNotCall());
       uni.write(unidata[0], common.mustCall());
       uni.end(unidata[1], common.mustCall());
       uni.on('finish', common.mustCall());
-      uni.on('end', common.mustCall());
-      uni.on('data', common.mustNotCall());
       uni.on('close', common.mustCall(() => {
         assert.strictEqual(uni.finalSize, 0);
       }));

diff --git a/test/parallel/test-quic-quicsession-send-fd.js b/test/parallel/test-quic-quicsession-send-fd.js
@@ -34,10 +34,13 @@ async function test({ variant, offset, length }) {
     session.on('secure', common.mustCall((servername, alpn, cipher) => {
       const stream = session.openStream({ halfOpen: true });
 
+      // The data and end events won't emit because
+      // the stream is never readable.
       stream.on('data', common.mustNotCall());
+      stream.on('end', common.mustNotCall());
+
       stream.on('finish', common.mustCall());
       stream.on('close', common.mustCall());
-      stream.on('end', common.mustCall());
 
       if (variant === 'sendFD') {
         fd = fs.openSync(__filename, 'r');