Skip to content
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

Implement flow-control in TransformStream fixes #330 maybe #429

Closed
wants to merge 2 commits into from

Conversation

asutherland
Copy link

Promise-ify the TransformStream so that it always waits for both a
write() and a pull() to have happened, returning a Promise in both
cases so that callers can know when it's safe to call again.

Previously pull() had no effect and the TransformStream was strictly
pumped by its write() callers.

NB: I may be making a dangerous assumption about pull() always being called. I will endeavor to better come up-to-speed on the spec and reference implementation and investigate this on my own in the next ~1 week. But in the meantime, I figured I'd put this here since it does pass the existing tests and the test I added that's only slightly pyramid-of-doomy.

Promise-ify the TransformStream so that it always waits for both a
write() and a pull() to have happened, returning a Promise in both
cases so that callers can know when it's safe to call again.

Previously pull() had no effect and the TransformStream was strictly
pumped by its write() callers.
@asutherland
Copy link
Author

It appears I did make a bad assumption about pull() being called whenever the previous pull() promise resolves. If the transformation function is something cool like a filtering function that does not always enqueue a result, then RequestReadableStreamPull as invoked by ReadFromReadableStreamReader will not reissue the pull, breaking stream flow.

I suspect reissuing the pull may make infinite loops quite likely, suggesting my patch here that tries to be clever with Promise pairing is not the way. I think I better understand the discussion on #330 now and will look into using ready or the propagation of the write result if I don't hear better suggestions here or on that bug.

I'm going to leave this PR open for now since I have a test and am about to push my follow-up test that demonstrates why my current PR is no good and plan to try and provide a better patch tomorrow. But I won't take offense if you close this until I have something that works! :)

@domenic
Copy link
Member

domenic commented Feb 10, 2016

I think it's fine to leave this open as a reminder that transform streams are not in great shape right now :). First we need to update writable stream (#318 and #319) and then look into fixing all the outstanding transform stream issues. Thanks for working on it though!

@domenic
Copy link
Member

domenic commented Aug 4, 2016

Going to close this in preparation for a transform stream overhaul :)

@domenic domenic closed this Aug 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants