-
Notifications
You must be signed in to change notification settings - Fork 161
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
ReadableStream.from(asyncIterable) #1083
Changes from all commits
13f2911
c946567
9b72815
f931fc2
5c48420
8e151d6
0a8f260
d065c85
991477e
0d07b3d
99b858d
76080da
ca0daf7
0724c91
523f3d0
7b8db87
078a335
3337ff7
5275f3b
1796989
d016390
bf5f360
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,8 @@ const assert = require('assert'); | |
const { promiseResolvedWith, promiseRejectedWith, newPromise, resolvePromise, rejectPromise, uponPromise, | ||
setPromiseIsHandledToTrue, waitForAllPromise, transformPromiseWith, uponFulfillment, uponRejection } = | ||
require('../helpers/webidl.js'); | ||
const { CanTransferArrayBuffer, CopyDataBlockBytes, CreateArrayFromList, IsDetachedBuffer, TransferArrayBuffer } = | ||
require('./ecmascript.js'); | ||
const { CanTransferArrayBuffer, Call, CopyDataBlockBytes, CreateArrayFromList, GetIterator, GetMethod, IsDetachedBuffer, | ||
IteratorComplete, IteratorNext, IteratorValue, TransferArrayBuffer, typeIsObject } = require('./ecmascript.js'); | ||
const { CloneAsUint8Array, IsNonNegativeNumber } = require('./miscellaneous.js'); | ||
const { EnqueueValueWithSize, ResetQueue } = require('./queue-with-sizes.js'); | ||
const { AcquireWritableStreamDefaultWriter, IsWritableStreamLocked, WritableStreamAbort, | ||
|
@@ -55,6 +55,7 @@ Object.assign(exports, { | |
ReadableStreamDefaultControllerHasBackpressure, | ||
ReadableStreamDefaultReaderRead, | ||
ReadableStreamDefaultReaderRelease, | ||
ReadableStreamFromIterable, | ||
ReadableStreamGetNumReadRequests, | ||
ReadableStreamHasDefaultReader, | ||
ReadableStreamPipeTo, | ||
|
@@ -1879,3 +1880,61 @@ function SetUpReadableByteStreamControllerFromUnderlyingSource( | |
stream, controller, startAlgorithm, pullAlgorithm, cancelAlgorithm, highWaterMark, autoAllocateChunkSize | ||
); | ||
} | ||
|
||
function ReadableStreamFromIterable(asyncIterable) { | ||
let stream; | ||
const iteratorRecord = GetIterator(asyncIterable, 'async'); | ||
|
||
const startAlgorithm = () => undefined; | ||
|
||
function pullAlgorithm() { | ||
let nextResult; | ||
try { | ||
nextResult = IteratorNext(iteratorRecord); | ||
} catch (e) { | ||
return promiseRejectedWith(e); | ||
} | ||
const nextPromise = promiseResolvedWith(nextResult); | ||
return transformPromiseWith(nextPromise, iterResult => { | ||
if (!typeIsObject(iterResult)) { | ||
throw new TypeError('The promise returned by the iterator.next() method must fulfill with an object'); | ||
} | ||
const done = IteratorComplete(iterResult); | ||
if (done === true) { | ||
ReadableStreamDefaultControllerClose(stream._controller); | ||
} else { | ||
const value = IteratorValue(iterResult); | ||
ReadableStreamDefaultControllerEnqueue(stream._controller, value); | ||
} | ||
}); | ||
} | ||
|
||
function cancelAlgorithm(reason) { | ||
const iterator = iteratorRecord.iterator; | ||
let returnMethod; | ||
try { | ||
returnMethod = GetMethod(iterator, 'return'); | ||
} catch (e) { | ||
return promiseRejectedWith(e); | ||
} | ||
if (returnMethod === undefined) { | ||
return promiseResolvedWith(undefined); | ||
} | ||
let returnResult; | ||
try { | ||
returnResult = Call(returnMethod, iterator, [reason]); | ||
} catch (e) { | ||
return promiseRejectedWith(e); | ||
} | ||
const returnPromise = promiseResolvedWith(returnResult); | ||
return transformPromiseWith(returnPromise, iterResult => { | ||
if (!typeIsObject(iterResult)) { | ||
throw new TypeError('The promise returned by the iterator.return() method must fulfill with an object'); | ||
} | ||
return undefined; | ||
}); | ||
} | ||
|
||
stream = CreateReadableStream(startAlgorithm, pullAlgorithm, cancelAlgorithm, 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Setting the high-water mark to zero ensures In comparison, I think we want to follow the example of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. I have a slight bias toward keeping our default, since it must have been picked for a reason :). In particular, I believe our historical intention has been that by default readable streams should try to keep something in their internal queue, whether that something is derived from a Transform streams are a bit different, as we want introducing them to be more of a no-op and not cause a bunch of chunks to sit in intermediate transform-stream queues, unless explicitly requested. On the other hand, you could view There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh right, I didn't update the spec text yet to pass a zero high-water mark. But I'll first wait until we decide what that high-water mark should be. 😉
Hmm... it doesn't look like that's working as intended? 😕 By default, a var rs1 = new ReadableStream({
pull(c) {
console.log('pull');
c.enqueue('a');
}
}, { highWaterMark: 0 });
// no "pull" is logged yet
var rs2 = rs1.pipeThrough(new TransformStream());
// logs "pull" once There's also no good way to fix this. You can't set both the readable and writable HWM to 0, since then the pipe will stall: var rs1 = new ReadableStream({
pull(c) {
console.log('pull');
c.enqueue('a');
}
}, { highWaterMark: 0 });
var rs2 = rs1.pipeThrough(new TransformStream({}, { highWaterMark: 0 }));
var r = rs2.getReader();
await r.read(); // never resolves There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels like there's a connection missing somewhere... There's no way for the writable end of a transform stream to know that the readable end is being read from. 🤔 let { readable, writable } = new TransformStream({}, { highWaterMark: 0 });
let r = readable.getReader();
let w = writable.getWriter();
// we start reading from the readable end...
let read1 = r.read();
// ...but the writable end is still applying backpressure. :-/
w.desiredSize; // still 0
w.ready; // still pending There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. As you observed, a writable stream with a HWM of 0 will always have backpressure. So adding an identity TransformStream to a pipe can't be a complete no-op: it always increases the total queue size by 1.
In implementation practice we seem to be setting readable HWM to 0 whenever we create a platform stream, because it permits maximum control over backpressure. So I'm not sure the default of 1 is correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another reason for setting HWM to 0: you can treat const it1 = ['a', 'b', 'c'];
// This async iterator should behave the same as the original
const it2 = ReadableStream.from(it1).values();
for await (const chunk of it2) { /* ... */ } const rs1 = new ReadableStream({
start(c) {
c.enqueue('a');
c.enqueue('b');
c.enqueue('c');
c.close();
}
});
// This stream should behave the same as the original
const rs2 = ReadableStream.from(rs1.values());
const reader = rs2.getReader();
for (let result = await reader.read(); !result.done; result = await reader.read()) { /* ... */ } |
||
return stream; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be excellent to see this move forward. With regards to the question about the special case for byte sources, the signature here can echo the signature for the
new ReadableStream()
constructor...That is, make the first argument either a dictionary or an iterator...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel like there's much precedent for an API that accepts two different types of objects (
Iterable or FromInit
). What should happen if you pass an object that is bothIterable
andFromInit
?What's the
autoAllocateChunkSize
for? We don't really have a way to expose the BYOB request to the async iterable. Or do you expect it to be passed as an argument tonext()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine. It was largely just a strawman anyway. I'm just as happy deferring any support for BYOB in this mechanism.