WritableStream: clear algorithms once they will no longer be called #940
Conversation
There was a problem hiding this comment.
This approach will work. I wish it were a bit more elegant, though.
In particular, I'm surprised the controller needs to be told when we're "finished" with it, via [FinishSteps]. It's also a bit uncomfortable that with that name, it's only called sometimes.
In the two cases, i.e. steps 9 and 12 of WritableStreamFinishedErroring---is it possible there was an earlier point where the controller could know that its algorithms are clearable?
Or alternately, I think this would work as a different version of WritableStreamFinishedErroring:
- Get rid of [[FinishSteps]]
- Make [[ErrorSteps]] clear the algorithms. (Kinda fits; it already clears the queue.)
- Instead of calling [[ErrorSteps]] at the beginning, call it at the end: i.e., right before the three return points of the algorithm.
WDYT?
index.bs
Outdated
| 1. Let _storedError_ be _stream_.[[storedError]]. | ||
| 1. Repeat for each _writeRequest_ that is an element of _stream_.[[writeRequests]], | ||
| 1. <a>Reject</a> _writeRequest_ with _storedError_. | ||
| 1. Set _stream_.[[writeRequests]] to an empty List. | ||
| 1. if _stream_.[[pendingAbortRequest]] is *undefined*, | ||
| 1. Perform ! _controller_[[FinishSteps]](). |
index.bs
Outdated
| @@ -3291,21 +3291,24 @@ nothrow>WritableStreamDealWithRejection ( <var>stream</var>, <var>error</var> )< | |||
| 1. Assert: _stream_.[[state]] is `"erroring"`. | |||
| 1. Assert: ! WritableStreamHasOperationMarkedInFlight(_stream_) is *false*. | |||
| 1. Set _stream_.[[state]] to `"errored"`. | |||
| 1. Perform ! _stream_.[[writableStreamController]].[[ErrorSteps]](). | |||
| 1. Let _controller_ be _stream_.[[writableStreamController]]. | |||
| 1. Perform ! _controller_.[[ErrorSteps]](). | |||
| 1. Let _storedError_ be _stream_.[[storedError]]. | |||
| 1. Repeat for each _writeRequest_ that is an element of _stream_.[[writeRequests]], | |||
| 1. <a>Reject</a> _writeRequest_ with _storedError_. | |||
| 1. Set _stream_.[[writeRequests]] to an empty List. | |||
| 1. if _stream_.[[pendingAbortRequest]] is *undefined*, | |||
There was a problem hiding this comment.
While here, maybe fix this lowercase "if"
Clear the [[writeAlgorithm]], [[closeAlgorithm]] and [[abortAlgorithm]] slots when they will no longer be called. This allows resources such as the underlying sink object to be freed. Add [[FinishSteps]] polymorphic operation so that the algorithms will be cleared correctly even for a future controller that has different slots. [[FinishSteps]] is only used when the clear operation is initiated from the WritableStream -- in the case of abort() and close() the clear operation is initiated by the controller itself and doesn't indirect via [[FinishSteps]]. Part of whatwg#932.
Avoid having polymorphic calls by triggering algo resets from the controller code. This can lead to redundant calls to ClearAlgorithms, but they are harmless.
ricea
force-pushed
the
writable-clear-algorithms
branch
from
4b63003
to
c8fee18
Compare
I managed to get this working. PTAL. |
index.bs
Outdated
| adds <a href="https://github.com/tc39/proposal-weakrefs/">weak references</a>. But even without that factor, | ||
| implementations will likely want to include similar steps.</p> | ||
|
|
||
| <p class="note">This operation will be performed multiple times in some edge cases. After the first time it will do nothing.</p> |
| @@ -919,6 +929,9 @@ function WritableStreamDefaultControllerProcessWrite(controller, chunk) { | |||
| WritableStreamDefaultControllerAdvanceQueueIfNeeded(controller); | |||
| }, | |||
| reason => { | |||
| if (stream._state === 'writable') { | |||
There was a problem hiding this comment.
This check does not appear in the spec text. I believe it should be removed from the reference implementation?
There was a problem hiding this comment.
Great catch! Actually this check is required, and I failed to add it to the spec text. Without this check, if a write() rejects while an abort() is already in progress, the abortAlgorithm will be cleared too early and so when the time comes to call the underlying source abort() it will fail.
Clear the [[writeAlgorithm]], [[closeAlgorithm]] and [[abortAlgorithm]]
slots when they will no longer be called. This allows resources such as
the underlying sink object to be freed.
Add [[FinishSteps]] polymorphic operation so that the algorithms will be
cleared correctly even for a future controller that has different slots.
[[FinishSteps]] is only used when the clear operation is initiated from
the WritableStream -- in the case of abort() and close() the clear
operation is initiated by the controller itself and doesn't indirect via
[[FinishSteps]].
Part of #932.
Preview | Diff