-
Notifications
You must be signed in to change notification settings - Fork 29.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
byob reader support for Blob.stream() #47993
Comments
Ok interestingly if we call |
Not really we have to change the way we respond too no? I was trying out something like this I was trying something like this diff --git a/lib/internal/blob.js b/lib/internal/blob.js
index ee8e1c7581..47face7a93 100644
--- a/lib/internal/blob.js
+++ b/lib/internal/blob.js
@@ -321,6 +321,7 @@ class Blob {
const reader = this[kHandle].getReader();
return new lazyReadableStream({
+ type: 'bytes',
start(c) {
// There really should only be one read at a time so using an
// array here is purely defensive.
@@ -353,7 +354,20 @@ class Blob {
return;
}
if (buffer !== undefined) {
- c.enqueue(new Uint8Array(buffer));
+ if (c.byobRequest) {
+ const byobRequestView = c.byobRequest.view;
+ const byteLength = buffer.byteLength;
+ const viewByteLength = byobRequestView.byteLength;
+ const viewByteOffset = byobRequestView.byteOffset;
+ for (let i = 0; i < byteLength;) {
+ const chunk = new Uint8Array(buffer.slice(i, i + viewByteLength));
+ byobRequestView.set(chunk, viewByteOffset);
+ i += viewByteLength;
+ c.byobRequest.respond(viewByteLength);
+ }
+ } else {
+ c.enqueue(new Uint8Array(buffer));
+ }
}
pending.resolve();
}); but this causes some error with detached array buffers which i dont yet understand 😅 |
When you enqueue a uint8array then the underlying ArrayBuffer gets detached. ab = new Uint8Array([97, 98]).buffer
chunk1 = new Uint8Array(ab, 0, 1)
chunk2 = new Uint8Array(ab, 1, 1)
rs = new ReadableStream({
type: 'bytes',
start(ctrl) {
console.log(chunk1.byteLength, ab.byteLength) // 1, 2
ctrl.enqueue(chunk1)
console.log(chunk1.byteLength, ab.byteLength) // 0, 0
// if you now try to enqueue chunk2 then it will fail
// ctrl.enqueue(chunk2)
ctrl.close()
}
})
into = new Uint8Array(2)
rs.getReader({mode: 'byob'}).read(into) It's as if you have transfered the arraybuffer to another thread using |
Ah understood so what could be a work around for the byob mode? |
Haven't investigated the source code so much. so if you are for some reason reusing the same ArrayBuffer twice then that might be an issue. anyhow. i'm not sure if you should enqueue a uint8array anyway. i suppose you are going to use the stream spec have a good turtorial on how to use files + byte streams here const fs = require("fs").promises;
const DEFAULT_CHUNK_SIZE = 1024;
function makeReadableByteFileStream(filename) {
let fileHandle;
let position = 0;
return new ReadableStream({
type: "bytes",
async start() {
fileHandle = await fs.open(filename, "r");
},
async pull(controller) {
// Even when the consumer is using the default reader, the auto-allocation
// feature allocates a buffer and passes it to us via byobRequest.
const v = controller.byobRequest.view;
const { bytesRead } = await fileHandle.read(v, 0, v.byteLength, position);
if (bytesRead === 0) {
await fileHandle.close();
controller.close();
controller.byobRequest.respond(0);
} else {
position += bytesRead;
controller.byobRequest.respond(bytesRead);
}
},
cancel() {
return fileHandle.close();
},
autoAllocateChunkSize: DEFAULT_CHUNK_SIZE
});
} |
Ah got it! i think responding multiple times by slicing the array buffer wont work, i think this would need changes on c++ side too, trying to investigate thank you for the inputs!! |
I don't see why slicing the arraybuffer would not work. you are creating a copy after all. but doing so this way would probably also indicate that you probably are also are doing something wrong in the first place... it would be better to read into an existing arraybuffer view instead |
Oh ok @KhafraDev was right it seems to be as simple as adding type: 'bytes' heh I went of on unnecessary tangent but it seems the promise hang issue is what needs to be looked into |
i tough so too. |
Hey so I opened a PR to fix the hanging issue I pointed out in #47993 (comment) here #48232 but as for adding 'bytes' support there seems like we have another issue #48233 |
Refs: #47993 (comment) PR-URL: #48232 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Refs: #47993 (comment) PR-URL: #48232 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Refs: nodejs#47993 (comment) PR-URL: nodejs#48232 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Refs: nodejs#47993 (comment) PR-URL: nodejs#48232 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
Fixes: nodejs#47993 PR-URL: nodejs#49713 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Version
all
Platform
No response
Subsystem
No response
What steps will reproduce the bug?
How often does it reproduce? Is there a required condition?
every time
What is the expected behavior? Why is that the expected behavior?
get a byob reader back
What do you see instead?
Additional information
Works in Firefox, Deno
stream()
? w3c/FileAPI#186The text was updated successfully, but these errors were encountered: