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

pipeTo says "in parallel", but that isn't really right #905

Open
domenic opened this Issue Mar 9, 2018 · 0 comments

Comments

1 participant
@domenic
Member

domenic commented Mar 9, 2018

(Previously briefly discussed in #902.)

The current pipeTo() spec says to run the reading/writing stuff "in parallel", i.e. on a background thread. This is wrong, mainly because of the case of developer-created streams, where reading and writing necessarily means running JS code.

The intent here is to not block the main thread while reading/writing. This is generally automatic if you use the existing read/write operations, as those all return promises. But, we wanted to allow some flexibility in the unobservable cases, so we just used vague "reading and writing", and from there my shortcut for saying "don't block" was "do this in parallel". This has other subpar consequences; see #618 and #728.

Currently I am thinking we want to more clearly suggest that you use the spec's existing read/write operations, and clarify what is and isn't observable. (Is: calling into JS-created underlying sources/sinks. Isn't: a bunch of other stuff.) And then remove all mention of "in parallel".

I'm not sure exactly how we should phrase things here, and whether it'll cover all task queuing/non-blocking/realm related stuff, but I'll try to give it a shot. In the meantime, I want this issue as documentation of a known problem, which we can link to in the spec.

@domenic domenic added the piping label Mar 9, 2018

@domenic domenic self-assigned this Mar 9, 2018

domenic added a commit that referenced this issue Mar 9, 2018

Clarify that objects created during pipeTo() are not observable
Closes #845. Although, in the process of writing this we discovered #905. This commit also modifies the spec to highlight that issue, in the nearby text.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment