Support ReadableByteStreamController in stream piping#36497
Draft
Taym95 wants to merge 2 commits intoservo:mainfrom
Draft
Support ReadableByteStreamController in stream piping#36497Taym95 wants to merge 2 commits intoservo:mainfrom
Taym95 wants to merge 2 commits intoservo:mainfrom
Conversation
|
🔨 Triggering try run (#14428902729) for Linux (WPT) |
Taym95
force-pushed
the
Support-ReadableByteStreamController-in-stream-piping
branch
from
191aa13 to
e7f463c
Compare
|
|
Taym95
force-pushed
the
Support-ReadableByteStreamController-in-stream-piping
branch
from
e7f463c to
b4ec9cd
Compare
|
🔨 Triggering try run (#14428998306) for Linux (WPT) |
Taym95
force-pushed
the
Support-ReadableByteStreamController-in-stream-piping
branch
from
b4ec9cd to
692c7fa
Compare
|
Test results for linux-wpt from try job (#14428998306): Flaky unexpected result (19)
Stable unexpected results that are known to be intermittent (14)
|
|
✨ Try run (#14428998306) succeeded. |
Taym95
commented
Apr 13, 2025
gterzian
requested changes
Apr 14, 2025
Member
gterzian
left a comment
gterzian
left a comment
There was a problem hiding this comment.
Thanks for this.
I think it's a good idea to add a simple test for this in https://github.com/servo/servo/tree/d46a17a487ce7799fc3d1e60fa093c628e62869a/tests/wpt/mozilla/tests/mozilla
| pub(crate) fn read(&self, can_gc: CanGc) -> Rc<Promise> { | ||
| match self { | ||
| ReaderType::BYOB(_) => { | ||
| unreachable!("Native reading of a chunk can only be done with a default reader.") |
Member
Author
There was a problem hiding this comment.
this is what read_a_chuk does, byob read function requires a view.
Member
There was a problem hiding this comment.
What I mean is that here, unlike at read_a_chunk, the point is to actually use a BYOB reader for the native reading of chunk as part of the piping flow. And doing so will hit this branch of logic, and panic. Is my understanding correct?
Signed-off-by: Taym Haddadi <[email protected]>
Taym95
force-pushed
the
Support-ReadableByteStreamController-in-stream-piping
branch
from
692c7fa to
4ed54ee
Compare
Signed-off-by: Taym Haddadi <[email protected]>
|
🔨 Triggering try run (#14479051535) for Linux (WPT) |
Member
Author
Added test. |
|
Test results for linux-wpt from try job (#14479051535): Flaky unexpected result (22)
Stable unexpected results that are known to be intermittent (14)
|
|
✨ Try run (#14479051535) succeeded. |
gterzian
requested changes
Apr 16, 2025
| pub(crate) fn read(&self, can_gc: CanGc) -> Rc<Promise> { | ||
| match self { | ||
| ReaderType::BYOB(_) => { | ||
| unreachable!("Native reading of a chunk can only be done with a default reader.") |
Member
There was a problem hiding this comment.
What I mean is that here, unlike at read_a_chunk, the point is to actually use a BYOB reader for the native reading of chunk as part of the piping flow. And doing so will hit this branch of logic, and panic. Is my understanding correct?
| return promise_rejects_js(t, TypeError, rs.pipeTo(ws)).then(() => { | ||
| assert_false(ws.locked, 'the WritableStream must remain unlocked after pipeTo rejection'); | ||
| }); | ||
| }, 'pipeTo must fail if the ReadableStream is locked with a BYOB reader, and must not lock the WritableStream'); |
Member
There was a problem hiding this comment.
I think the logic of those two tests is already covered in the main suite, and what we need instead is a simply tests that actually enqueues chunks and pipes them successfully(I believe such a test would crash with the current implementation, so it's actually a good idea to write the test first and check that this is a correct assumption).
Member
Author
|
I will make this one a draft and address it later. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #34676, Fix #36013