src: some minor quic stream cleanups

Author: jasnell

src/quic/defs.h

@@ -309,15 +309,16 @@ enum class DatagramStatus : uint8_t {
 CC_ALGOS(V)
 #undef V
 
+using error_code = uint64_t;
+using stream_id = int64_t;
+using datagram_id = uint64_t;
+
 constexpr size_t kDefaultMaxPacketLength = NGTCP2_MAX_UDP_PAYLOAD_SIZE;
 constexpr uint64_t kMaxSizeT = std::numeric_limits<size_t>::max();
 constexpr uint64_t kMaxSafeJsInteger = 9007199254740991;
 constexpr auto kSocketAddressInfoTimeout = 60 * NGTCP2_SECONDS;
 constexpr size_t kMaxVectorCount = 16;
-
-using error_code = uint64_t;
-using stream_id = int64_t;
-using datagram_id = uint64_t;
+constexpr size_t kMaxStreamId = std::numeric_limits<stream_id>::max();
 
 class DebugIndentScope final {
  public:

src/quic/streams.cc

@@ -35,7 +35,7 @@ using v8::Value;
 namespace quic {
 
 #define STREAM_STATE(V)                                                        \
-  V(ID, id, int64_t)                                                           \
+  V(ID, id, stream_id)                                                         \
   V(PENDING, pending, uint8_t)                                                 \
   V(FIN_SENT, fin_sent, uint8_t)                                               \
   V(FIN_RECEIVED, fin_received, uint8_t)                                       \
@@ -107,7 +107,7 @@ PendingStream::~PendingStream() {
   }
 }
 
-void PendingStream::fulfill(int64_t id) {
+void PendingStream::fulfill(stream_id id) {
   CHECK(waiting_);
   waiting_ = false;
   stream_->NotifyStreamOpened(id);
@@ -145,31 +145,76 @@ Maybe<std::shared_ptr<DataQueue>> Stream::GetDataQueueFromSource(
   DCHECK_IMPLIES(!value->IsUndefined(), value->IsObject());
   std::vector<std::unique_ptr<DataQueue::Entry>> entries;
   if (value->IsUndefined()) {
+    // Return an empty DataQueue.
     return Just(std::shared_ptr<DataQueue>());
   } else if (value->IsArrayBuffer()) {
+    // DataQueue is created from an ArrayBuffer.
     auto buffer = value.As<ArrayBuffer>();
+    // We require that the ArrayBuffer be detachable. This ensures that the
+    // underlying memory can be transferred to the DataQueue without risk
+    // of the memory being modified by JavaScript code while it is owned
+    // by the DataQueue.
+    if (!buffer->IsDetachable()) {
+      THROW_ERR_INVALID_ARG_TYPE(env, "Data source not detachable");
+      return Nothing<std::shared_ptr<DataQueue>>();
+    }
+    auto backing = buffer->GetBackingStore();
+    uint64_t offset = 0;
+    uint64_t length = buffer->ByteLength();
+    if (buffer->Detach(Local<Value>()).IsNothing()) {
+      THROW_ERR_INVALID_ARG_TYPE(env, "Data source not detachable");
+      return Nothing<std::shared_ptr<DataQueue>>();
+    }
     entries.push_back(DataQueue::CreateInMemoryEntryFromBackingStore(
-        buffer->GetBackingStore(), 0, buffer->ByteLength()));
+        std::move(backing), offset, length));
     return Just(DataQueue::CreateIdempotent(std::move(entries)));
   } else if (value->IsSharedArrayBuffer()) {
-    auto buffer = value.As<SharedArrayBuffer>();
-    entries.push_back(DataQueue::CreateInMemoryEntryFromBackingStore(
-        buffer->GetBackingStore(), 0, buffer->ByteLength()));
-    return Just(DataQueue::CreateIdempotent(std::move(entries)));
+    // We aren't going to allow use of SharedArrayBuffer as a data source.
+    // The reason is that SharedArrayBuffer memory is possibly shared with
+    // other JavaScript code and we cannot detach it, making it impossible
+    // for us to guarantee that the memory will not be modified while it
+    // is owned by the DataQueue.
+    THROW_ERR_INVALID_ARG_TYPE(env, "SharedArrayBuffer is not allowed");
+    return Nothing<std::shared_ptr<DataQueue>>();
   } else if (value->IsArrayBufferView()) {
-    auto entry =
-        DataQueue::CreateInMemoryEntryFromView(value.As<ArrayBufferView>());
-    if (!entry) {
+    auto view = value.As<ArrayBufferView>();
+    auto buffer = view->Buffer();
+    if (buffer->IsSharedArrayBuffer()) {
+      // We aren't going to allow use of SharedArrayBuffer as a data source.
+      // The reason is that SharedArrayBuffer memory is possibly shared with
+      // other JavaScript code and we cannot detach it, making it impossible
+      // for us to guarantee that the memory will not be modified while it
+      // is owned by the DataQueue.
+      THROW_ERR_INVALID_ARG_TYPE(env, "SharedArrayBuffer is not allowed");
+      return Nothing<std::shared_ptr<DataQueue>>();
+    }
+    if (!buffer->IsDetachable()) {
       THROW_ERR_INVALID_ARG_TYPE(env, "Data source not detachable");
       return Nothing<std::shared_ptr<DataQueue>>();
     }
-    entries.push_back(std::move(entry));
+    if (buffer->Detach(Local<Value>()).IsNothing()) {
+      THROW_ERR_INVALID_ARG_TYPE(env, "Data source not detachable");
+      return Nothing<std::shared_ptr<DataQueue>>();
+    }
+    auto backing = buffer->GetBackingStore();
+    auto offset = view->ByteOffset();
+    auto length = view->ByteLength();
+    entries.push_back(DataQueue::CreateInMemoryEntryFromBackingStore(
+        std::move(backing), offset, length));
     return Just(DataQueue::CreateIdempotent(std::move(entries)));
   } else if (Blob::HasInstance(env, value)) {
     Blob* blob;
     ASSIGN_OR_RETURN_UNWRAP(
         &blob, value, Nothing<std::shared_ptr<DataQueue>>());
     return Just(blob->getDataQueue().slice(0));
+  } else if (value->IsString()) {
+    Utf8Value str(env->isolate(), value);
+    JS_TRY_ALLOCATE_BACKING_OR_RETURN(
+        env, backing, str.length(), Nothing<std::shared_ptr<DataQueue>>());
+    memcpy(backing->Data(), *str, str.length());
+    entries.push_back(DataQueue::CreateInMemoryEntryFromBackingStore(
+        std::move(backing), 0, backing->ByteLength()));
+    return Just(DataQueue::CreateIdempotent(std::move(entries)));
   }
   // TODO(jasnell): Add streaming sources...
   THROW_ERR_INVALID_ARG_TYPE(env, "Invalid data source type");
@@ -182,15 +227,16 @@ struct Stream::Impl {
   // Attaches an outbound data source to the stream.
   JS_METHOD(AttachSource) {
     Environment* env = Environment::GetCurrent(args);
+    Stream* stream;
+    ASSIGN_OR_RETURN_UNWRAP(&stream, args.This());
 
     std::shared_ptr<DataQueue> dataqueue;
     if (GetDataQueueFromSource(env, args[0]).To(&dataqueue)) {
-      Stream* stream;
-      ASSIGN_OR_RETURN_UNWRAP(&stream, args.This());
       stream->set_outbound(std::move(dataqueue));
     }
   }
 
+  // Immediately and forcefully destroys the stream.
   JS_METHOD(Destroy) {
     Stream* stream;
     ASSIGN_OR_RETURN_UNWRAP(&stream, args.This());
@@ -204,6 +250,10 @@ struct Stream::Impl {
     }
   }
 
+  // Sends a block of headers to the peer. If the stream is not yet open,
+  // the headers will be queued and sent immediately when the stream is
+  // opened. If the application does not support sending headers on streams,
+  // they will be ignored and dropped on the floor.
   JS_METHOD(SendHeaders) {
     Stream* stream;
     ASSIGN_OR_RETURN_UNWRAP(&stream, args.This());
@@ -233,7 +283,7 @@ struct Stream::Impl {
   JS_METHOD(StopSending) {
     Stream* stream;
     ASSIGN_OR_RETURN_UNWRAP(&stream, args.This());
-    uint64_t code = 0;
+    error_code code = 0;
     CHECK_IMPLIES(!args[0]->IsUndefined(), args[0]->IsBigInt());
     if (!args[0]->IsUndefined()) {
       bool unused = false;  // not used but still necessary.
@@ -258,7 +308,7 @@ struct Stream::Impl {
   JS_METHOD(ResetStream) {
     Stream* stream;
     ASSIGN_OR_RETURN_UNWRAP(&stream, args.This());
-    uint64_t code = 0;
+    error_code code = 0;
     CHECK_IMPLIES(!args[0]->IsUndefined(), args[0]->IsBigInt());
     if (!args[0]->IsUndefined()) {
       bool lossless = false;  // not used but still necessary.
@@ -315,6 +365,8 @@ struct Stream::Impl {
     args.GetReturnValue().Set(static_cast<uint32_t>(priority));
   }
 
+  // Returns a Blob::Reader that can be used to read data that has been
+  // received on the stream.
   JS_METHOD(GetReader) {
     Stream* stream;
     ASSIGN_OR_RETURN_UNWRAP(&stream, args.This());
@@ -758,7 +810,7 @@ Stream* Stream::From(void* stream_user_data) {
 }
 
 BaseObjectPtr<Stream> Stream::Create(Session* session,
-                                     int64_t id,
+                                     stream_id id,
                                      std::shared_ptr<DataQueue> source) {
   DCHECK_GE(id, 0);
   DCHECK_NOT_NULL(session);
@@ -778,7 +830,7 @@ BaseObjectPtr<Stream> Stream::Create(Session* session,
 
 Stream::Stream(BaseObjectWeakPtr<Session> session,
                Local<Object> object,
-               int64_t id,
+               stream_id id,
                std::shared_ptr<DataQueue> source)
     : AsyncWrap(session->env(), object, PROVIDER_QUIC_STREAM),
       stats_(env()->isolate()),
@@ -787,6 +839,7 @@ Stream::Stream(BaseObjectWeakPtr<Session> session,
       inbound_(DataQueue::Create()),
       headers_(env()->isolate()) {
   MakeWeak();
+  DCHECK(id < kMaxStreamId);
   state_->id = id;
   state_->pending = 0;
   // Allows us to be notified when data is actually read from the
@@ -818,7 +871,7 @@ Stream::Stream(BaseObjectWeakPtr<Session> session,
           std::make_unique<PendingStream>(direction, this, session_)),
       headers_(env()->isolate()) {
   MakeWeak();
-  state_->id = -1;
+  state_->id = kMaxStreamId;
   state_->pending = 1;
 
   // Allows us to be notified when data is actually read from the
@@ -841,8 +894,9 @@ Stream::~Stream() {
   DCHECK_NE(stats_->destroyed_at, 0);
 }
 
-void Stream::NotifyStreamOpened(int64_t id) {
+void Stream::NotifyStreamOpened(stream_id id) {
   CHECK(is_pending());
+  DCHECK(id < kMaxStreamId);
   Debug(this, "Pending stream opened with id %" PRIi64, id);
   state_->pending = 0;
   state_->id = id;
@@ -886,13 +940,13 @@ void Stream::NotifyStreamOpened(int64_t id) {
   if (outbound_) session().ResumeStream(id);
 }
 
-void Stream::NotifyReadableEnded(uint64_t code) {
+void Stream::NotifyReadableEnded(error_code code) {
   CHECK(!is_pending());
   Session::SendPendingDataScope send_scope(&session());
   ngtcp2_conn_shutdown_stream_read(session(), 0, id(), code);
 }
 
-void Stream::NotifyWritableEnded(uint64_t code) {
+void Stream::NotifyWritableEnded(error_code code) {
   CHECK(!is_pending());
   Session::SendPendingDataScope send_scope(&session());
   ngtcp2_conn_shutdown_stream_write(session(), 0, id(), code);
@@ -910,7 +964,7 @@ bool Stream::is_pending() const {
   return state_->pending;
 }
 
-int64_t Stream::id() const {
+stream_id Stream::id() const {
   return state_->id;
 }
 

src/quic/streams.h

@@ -105,7 +105,13 @@ class PendingStream final {
 // data is buffered in memory makes it essential that the flow control for the
 // session and the stream are properly handled. For now, we are largely relying
 // on ngtcp2's default flow control mechanisms which generally should be doing
-// the right thing.
+// the right thing. From the JavaScript side, the application pushes data into
+// the stream's outbound queue and ngtcp2 pulls data from that queue as it is
+// able. The stream outbound has a high watermark. The JS side can choose to
+// continue writing data even after the high watermark is reached but this
+// risks using up large amounts of memory if the session is slow to send data
+// or the peer is slow to acknowledge receipt. The JavaScript side needs to
+// be aware of this risk and pay proper attention to the backpressure signals.
 //
 // A Stream may be in a fully closed state (No longer readable nor writable)
 // state but still have unacknowledged data in both the inbound and outbound
@@ -115,23 +121,29 @@ class PendingStream final {
 // (b) all queued data has been acknowledged.
 //
 // The Stream may be forcefully closed immediately using destroy(err). This
-// causes all queued outbound data and pending JavaScript writes are abandoned,
-// and causes the Stream to be immediately closed at the ngtcp2 level without
-// waiting for any outstanding acknowledgements. Keep in mind, however, that the
-// peer is not notified that the stream is destroyed and may attempt to continue
-// sending data and acknowledgements.
+// causes all queued outbound data to be cleared, pending JavaScript writes
+// to be abandoned, the Stream to be immediately closed at the ngtcp2 level
+// without waiting for any outstanding acknowledgements. Keep in mind, however,
+// that the peer is not notified that the stream is destroyed and may attempt
+// to continue sending data and acknowledgements until it is able to determine
+// that the stream is gone. Any data that has already been received and is in
+// the inbound queue is preserved and may be read by the application.
 //
 // QUIC streams in general do not have headers. Some QUIC applications, however,
-// may associate headers with the stream (HTTP/3 for instance).
+// may associate headers with the stream (HTTP/3 for instance). As a
+// convenience, the Stream class will hold onto these headers for the
+// application.
 //
 // Streams may be created in a pending state. This means that while the Stream
 // object is created, it has not yet been opened in ngtcp2 and therefore has
 // no official status yet. Certain operations can still be performed on the
-// stream object such as providing data and headers, and destroying the stream.
+// stream object such as providing data, adding headers, or destroying the
+// stream.
 //
 // When a stream is created the data source for the stream must be given.
 // If no data source is given, then the stream is assumed to not have any
-// outbound data. The data source can be fixed length or may support
+// outbound data. If the stream was created as bidirectional, the outbound
+// side will be closed. The data source can be fixed length or may support
 // streaming. What this means practically is, when a stream is opened,
 // you must already have a sense of whether that will provide data or
 // not. When in doubt, specify a streaming data source, which can produce
@@ -142,15 +154,21 @@ class Stream final : public AsyncWrap,
  public:
   using Header = NgHeaderBase<BindingData>;
 
+  // Acquire a DataQueue from the given value if it is valid. The return
+  // follows the typical V8 rules for Maybe types. If an error occurs,
+  // the Maybe will be empty and an exception will be set on the isolate.
   static v8::Maybe<std::shared_ptr<DataQueue>> GetDataQueueFromSource(
       Environment* env, v8::Local<v8::Value> value);
 
+  // The stream_user_data field is from ngtcp2 and will point to the
+  // Stream instance associated with the stream_id.
   static Stream* From(void* stream_user_data);
 
   JS_CONSTRUCTOR(Stream);
   JS_BINDING_INIT_BOILERPLATE();
 
-  // Creates a new non-pending stream.
+  // Creates a new non-pending stream. The directionality of the stream
+  // is inferred from the stream id.
   static BaseObjectPtr<Stream> Create(
       Session* session,
       stream_id id,
@@ -179,7 +197,8 @@ class Stream final : public AsyncWrap,
   DISALLOW_COPY_AND_MOVE(Stream)
   ~Stream() override;
 
-  // While the stream is still pending, the id will be -1.
+  // While the stream is still pending, the id will be kMaxStreamId,
+  // inidicating the maximum possible stream id is kMaxStreamId - 1.
   stream_id id() const;
 
   // While the stream is still pending, the origin will be invalid.
@@ -208,13 +227,23 @@ class Stream final : public AsyncWrap,
   // Called by the session/application to indicate that the specified number
   // of bytes have been acknowledged by the peer.
   void Acknowledge(size_t datalen);
+
+  // Called by the session/application to indicate that the specified number
+  // of bytes have been transmitted to the peer.  This is an initial
+  // indication occuring the first time data is sent. It does not indicate
+  // that the data has been retransmitted due to loss or has been
+  // acknowledged to have been received by the peer.
   void Commit(size_t datalen);
 
   void EndWritable();
   void EndReadable(std::optional<uint64_t> maybe_final_size = std::nullopt);
   void EntryRead(size_t amount) override;
 
   // Pulls data from the internal outbound DataQueue configured for this stream.
+  // This is called by the session/application when it is preparing to send
+  // data to the peer. There is no guarantee that the requested amount of data
+  // will actually be sent. The amount of data actually sent is indicated
+  // by the datalen argument to the Commit() method.
   int DoPull(bob::Next<ngtcp2_vec> next,
              int options,
              ngtcp2_vec* data,
@@ -282,7 +311,8 @@ class Stream final : public AsyncWrap,
   void EmitReset(const QuicError& error);
 
   // Notifies the JavaScript side that the application is ready to receive
-  // trailing headers.
+  // trailing headers. Any trailing headers must be sent immediately, and
+  // synchronously when this callback is triggered.
   void EmitWantTrailers();
 
   // Notifies the JavaScript side that sending data on the stream has been
@@ -292,8 +322,8 @@ class Stream final : public AsyncWrap,
   // Delivers the set of inbound headers that have been collected.
   void EmitHeaders();
 
-  void NotifyReadableEnded(uint64_t code);
-  void NotifyWritableEnded(uint64_t code);
+  void NotifyReadableEnded(error_code code);
+  void NotifyWritableEnded(error_code code);
 
   // When a pending stream is finally opened, the NotifyStreamOpened method
   // will be called and the id will be assigned.
@@ -314,8 +344,8 @@ class Stream final : public AsyncWrap,
   std::optional<std::unique_ptr<PendingStream>> maybe_pending_stream_ =
       std::nullopt;
   std::vector<std::unique_ptr<PendingHeaders>> pending_headers_queue_;
-  uint64_t pending_close_read_code_ = 0;
-  uint64_t pending_close_write_code_ = 0;
+  error_code pending_close_read_code_ = 0;
+  error_code pending_close_write_code_ = 0;
 
   struct PendingPriority {
     StreamPriority priority;