Mark promise rejections for "state promises" as handled #607
Conversation
687f850 to
7a5f2ee
Compare
|
|
||
| return promise_rejects(t, error1, writer.closed, 'writer.closed must reject with the write error') | ||
| return promise_rejects(t, error1, writer.write('Hello'), 'writer.write() must reject with the write error') | ||
| .then(() => { |
There was a problem hiding this comment.
How about doing without the {} here?
This fixes #547, by marking the state promises (reader.closed, writer.closed, and writer.ready) as handled whenever they are rejected. It also overhauls the tests to handle all rejections, as apparently this will be enforced by testharness.js soon: see w3c/testharness.js@7716e25. In the process, this adds additional test coverage for many rejections: since we have to handle them anyway, we might as well test them with promise_rejects. It also led to a fix to the completely-broken test "WritableStream should call underlying sink's close if no abort is supplied" in aborting.js. Finally, it uncovered an issue where ongoing write() calls would reject if abort() is called subsequently, even if the ongoing write succeeds according to the underlying sink.
7a5f2ee to
d4eb805
Compare
|
OK, this should be ready to go. Here is a proposed commit message: |
|
lgtm |
|
I found that the reference implementation will be readable and easier to maintain if we move the I attempted to do this refactoring, but noticed that considering that the purpose of the reference implementation is for checking the correctness of the specification, it might be better to keep Thoughts? If you agree, I just do nothing. |
|
@tyoshino I already had this conversation inside my head. Maybe I should have mentioned it to someone else. Anyway, I came to the same conclusion that you did: that it's important to do the steps in the same order and granularity as the standard, even if it sometimes makes the reference implementation a bit unnatural. |
|
@ricea Thanks. Then ok! |
This fixes #547, by marking the state promises (reader.closed, writer.closed, and writer.ready) as handled whenever they are rejected. It also overhauls the tests to handle all rejections, as apparently this will be enforced by testharness.js soon: see w3c/testharness.js@7716e25.
In the process, this adds additional test coverage for many rejections: since we have to handle them anyway, we might as well test them with promise_rejects. It also led to a fix to the completely-broken test "WritableStream should call underlying sink's close if no abort is supplied" in aborting.js. Finally, it uncovered an issue where ongoing write() calls would reject if abort() is called subsequently, even if the ongoing write succeeds according to the underlying sink.
This is not yet ready to merge as a test is still failing, for the write() + abort() case. It is the test "Aborting a WritableStream prevents further writes after any that are in progress", which I have updated to expect that the write() fulfills, but currently it fails with the aborted TypeError.
The problem is essentially that abort() synchronously rejects all pending write requests, whereas WritableStreamDefaultControllerProcessWrite does not unshift the pending write request until the underlying sink's write() call succeeds, which happens asynchronously.
We should instead unshift the pending write request the moment the underlying sink write() call is made, I think. This matches better the idea that once the underlying sink write() has started, it is entirely up to the underlying sink whether the writer.write() call should fulfill or reject. But a quick attempt to do that leads to lots of test timeouts and other failures. I would appreciate help investigating ways of making the test pass.
Alternately we could mark the test as skipped for now (I guess in WPT land that would mean commenting it out and filing an issue) just to get the promise rejection stuff underway.