Add TransformStream tests for re-entrant strategies #795
Conversation
Add tests to ensure that implementations of TransformStream behave correctly when operations are triggered from within the readableStrategy.size function. This is not really a "supported" use case as any code that does this is either malicious or broken, but implementations need to avoid crashes or memory corruption in these cases. Also merge changes to recording-streams.js from web-platform-tests that are required to make these tests work.
ricea
force-pushed
the
transform-stream-reentrant-strategies
branch
from
168e56c
to
4e970b0
Compare
| }); | ||
| reader = ts.readable.getReader(); | ||
| const writer = ts.writable.getWriter(); | ||
| let writeCalled = false; |
There was a problem hiding this comment.
how about naming this writeResolved, writeCompleted, etc?
There was a problem hiding this comment.
Great, thanks. Done.
| }).then(() => { | ||
| assert_true(writeCalled); | ||
| assert_equals(calls, 1, 'size() should only be called once'); | ||
| return readPromises[0]; |
There was a problem hiding this comment.
Yes, Travis found the bug just before you did. Thanks for checking anyway!
It is more explanatory.
|
It's very tiring to read these tests, so feel free to take your time reviewing them. They're not blocking anything else. |
|
|
||
| // The behaviour in this test may seem strange, but it is logical. The call to controller.close() happens while the | ||
| // readable queue is still empty, so the readable transitions to the "closed" state immediately, and the chunk is left | ||
| // stranded in the readable's queue. |
There was a problem hiding this comment.
stranded in the writable's queue, right?
There was a problem hiding this comment.
Nope, readable's queue. I've changed from one big comment to commenting the individual steps, which will hopefully make it clearer.
| assert_false(done, 'done should be false'); | ||
| // See https://github.com/whatwg/streams/issues/794 for why this chunk is not 'a'. | ||
| assert_equals(value, 'b', 'chunk should have been read'); | ||
| return writePromise; |
There was a problem hiding this comment.
Maybe throw in another assert on calls? It doesn't hurt... Same in next test.
Also add some extra checks for the number of times size() is called.
Add tests to ensure that implementations of TransformStream behave correctly
when operations are triggered from within the readableStrategy.size function.
This is not really a "supported" use case as any code that does this is either
malicious or broken, but implementations need to avoid crashes or memory
corruption in these cases.
Also merge changes to recording-streams.js from web-platform-tests that are
required to make these tests work.