Merge pull request #5776 from cloudflare/yagiz/fix-desiredSize

desiredSize should be null when the stream is erroring

Author: anonrig

src/workerd/api/streams/standard.c++

@@ -1130,7 +1130,9 @@ template <typename Self>
 WritableImpl<Self>::WritableImpl(
     jsg::Lock& js, WritableStream& owner, jsg::Ref<AbortSignal> abortSignal)
     : owner(owner.addWeakRef()),
-      signal(kj::mv(abortSignal)) {}
+      signal(kj::mv(abortSignal)) {
+  flags.pedanticWpt = FeatureFlags::get(js).getPedanticWpt();
+}
 
 template <typename Self>
 jsg::Promise<void> WritableImpl<Self>::abort(
@@ -3244,7 +3246,11 @@ void WritableStreamDefaultController::error(
   impl.error(js, JSG_THIS, reason.orDefault(js.undefined()));
 }
 
-ssize_t WritableStreamDefaultController::getDesiredSize() {
+kj::Maybe<ssize_t> WritableStreamDefaultController::getDesiredSize() {
+  // Per the spec, desiredSize should be null when the stream is erroring.
+  if (impl.flags.pedanticWpt && isErroring()) {
+    return kj::none;
+  }
   return impl.getDesiredSize();
 }
 
@@ -3403,7 +3409,7 @@ kj::Maybe<int> WritableStreamJsController::getDesiredSize() {
       return kj::none;
     }
     KJ_CASE_ONEOF(controller, Controller) {
-      return controller->getDesiredSize();
+      return controller->getDesiredSize().map([](ssize_t size) -> int { return size; });
     }
   }
   KJ_UNREACHABLE;
@@ -3416,6 +3422,10 @@ kj::Maybe<v8::Local<v8::Value>> WritableStreamJsController::isErroring(jsg::Lock
   return kj::none;
 }
 
+bool WritableStreamDefaultController::isErroring() const {
+  return impl.state.is<StreamStates::Erroring>();
+}
+
 kj::Maybe<v8::Local<v8::Value>> WritableStreamJsController::isErroredOrErroring(jsg::Lock& js) {
   KJ_IF_SOME(err, state.tryGet<StreamStates::Errored>()) {
     return err.getHandle(js);

src/workerd/api/streams/standard.h

@@ -385,6 +385,7 @@ class WritableImpl {
     uint8_t started : 1 = 0;
     uint8_t starting : 1 = 0;
     uint8_t backpressure : 1 = 0;
+    uint8_t pedanticWpt : 1 = 0;
   };
   Flags flags{};
 
@@ -597,12 +598,17 @@ class WritableStreamDefaultController: public jsg::Object {
 
   void error(jsg::Lock& js, jsg::Optional<v8::Local<v8::Value>> reason);
 
-  ssize_t getDesiredSize();
+  kj::Maybe<ssize_t> getDesiredSize();
 
   jsg::Ref<AbortSignal> getSignal();
 
   kj::Maybe<v8::Local<v8::Value>> isErroring(jsg::Lock& js);
 
+  // Returns true if the stream is in the erroring state. Unlike the overload
+  // that takes a lock, this method does not require a lock since it doesn't
+  // return the error reason.
+  bool isErroring() const;
+
   bool isStarted() {
     return impl.flags.started;
   }

src/workerd/api/streams/writable.c++

@@ -112,7 +112,7 @@ jsg::MemoizedIdentity<jsg::Promise<void>>& WritableStreamDefaultWriter::getClose
   return KJ_ASSERT_NONNULL(closedPromise, "the writer was never attached to a stream");
 }
 
-kj::Maybe<int> WritableStreamDefaultWriter::getDesiredSize(jsg::Lock& js) {
+kj::Maybe<int> WritableStreamDefaultWriter::getDesiredSize() {
   KJ_SWITCH_ONEOF(state) {
     KJ_CASE_ONEOF(i, Initial) {
       KJ_FAIL_ASSERT("this writer was never attached");

src/workerd/api/streams/writable.h

@@ -23,7 +23,7 @@ class WritableStreamDefaultWriter: public jsg::Object, public WritableStreamCont
 
   jsg::MemoizedIdentity<jsg::Promise<void>>& getClosed();
   jsg::MemoizedIdentity<jsg::Promise<void>>& getReady();
-  kj::Maybe<int> getDesiredSize(jsg::Lock& js);
+  kj::Maybe<int> getDesiredSize();
 
   jsg::Promise<void> abort(jsg::Lock& js, jsg::Optional<v8::Local<v8::Value>> reason);
 

src/wpt/streams-test.ts

@@ -745,11 +745,9 @@ export default {
     expectedFailures:
       process.platform === 'win32'
         ? [
-            'controller argument should be passed to start method',
             'WritableStream should be writable and ready should fulfill immediately if the strategy does not apply backpressure',
           ]
         : [
-            'controller argument should be passed to start method',
             'WritableStream should be writable and ready should fulfill immediately if the strategy does not apply backpressure',
             'underlyingSink argument should be converted after queuingStrategy argument',
           ],
@@ -770,7 +768,6 @@ export default {
   'writable-streams/general.any.js': {
     comment: 'To be investigated',
     expectedFailures: [
-      'desiredSize on a writer for an errored stream',
       "WritableStream's strategy.size should not be called as a method",
       'closed and ready on a released writer',
       'ready promise should fire before closed on releaseLock',