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

streams - request for docs: can _transform or _write(v) be called in parallel? #3208

Closed
ronkorving opened this issue Oct 6, 2015 · 8 comments
Labels
doc Issues and PRs related to the documentations. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. stream Issues and PRs related to the stream subsystem.

Comments

@ronkorving
Copy link
Contributor

The reason I ask is because I have a transform-stream that receives objects and turns those into a bunch of buffers for each single transform. Some of these buffers I have to push out always have the same length. If possible (safe) I would like to create those buffers in my transform stream constructor and simply stick new data in during each transform.

So my question: as long as I don't call the callback in _transform, is it safe to assume _transform will not be called again? If my transform is piped down to disk for example, is there a chance that me changing the buffer in the next _transform call could have an effect on the outcome of the first write to disk?

In either case, documentation on this would be most welcome. I think stream implementers (myself in this case) often really need to squeeze out all performance, and understanding how this behaves would help. Also, as long as it's not documented one way or the other, I would be scared to depend on the behavior (even if it behaves the way I hope it does), because it may change tomorrow.

@mscdex mscdex added the stream Issues and PRs related to the stream subsystem. label Oct 6, 2015
@brendanashworth
Copy link
Contributor

I'm fairly sure you're correct - this is a guarantee for streams. I'm surprised that it isn't documented, actually.

@bnoordhuis
Copy link
Member

/cc @nodejs/documentation? Provisionally adding a doc label.

@bnoordhuis bnoordhuis added doc Issues and PRs related to the documentations. docs-requested labels May 2, 2016
@stevemao
Copy link
Contributor

stevemao commented May 4, 2016

also CC @nodejs/streams

@mcollina
Copy link
Member

mcollina commented May 4, 2016

@ronkorving correct. For each stream there is only one _transform and _write and _writev is executed at any given time.

My module throughv does parallel transform (and it's compatible with streams3).

@addaleax addaleax added the good first issue Issues that are suitable for first-time contributors. label May 4, 2016
@sam-github sam-github added doc Issues and PRs related to the documentations. and removed doc Issues and PRs related to the documentations. docs-requested labels Dec 1, 2016
@Trott
Copy link
Member

Trott commented Jul 15, 2017

This has been open for quite some time with the good first contribution label. Long enough that I think it might be best if someone experienced just goes ahead and does it. Any volunteers?

@Trott Trott added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. and removed good first issue Issues that are suitable for first-time contributors. labels Jul 15, 2017
mcollina added a commit to mcollina/node that referenced this issue Jul 17, 2017
Add a note to the stream docs specifying that at most a single
call to _transform can happen, and the provided callback()
should be used to process another chunk.

Fixes: nodejs#3208
@mcollina
Copy link
Member

Fixed in 0e5283b.

mcollina added a commit that referenced this issue Jul 19, 2017
Add a note to the stream docs specifying that at most a single
call to _transform can happen, and the provided callback()
should be used to process another chunk.

Fixes: #3208
PR-URL: #14321
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@ronkorving
Copy link
Contributor Author

@mcollina Appreciate the documentation update. But it doesn't quite answer the question I had. Maybe my phrasing was simply unclear. So let me be more specific:

During every transform, I write a 10 byte buffer. The contents of that buffer will be different each time. Is it safe to reuse the same 10 byte Buffer instance each transform, or will the 2nd transform change the contents of the 1st transform?

My guess is that this is unsafe, but .. that's really where this question originated.

Apologies for the lack of clarity.

@mcollina
Copy link
Member

You cannot reuse buffers in any part of the streams API.

addaleax pushed a commit that referenced this issue Jul 22, 2017
Add a note to the stream docs specifying that at most a single
call to _transform can happen, and the provided callback()
should be used to process another chunk.

Fixes: #3208
PR-URL: #14321
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit that referenced this issue Jul 24, 2017
Add a note to the stream docs specifying that at most a single
call to _transform can happen, and the provided callback()
should be used to process another chunk.

Fixes: #3208
PR-URL: #14321
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
Add a note to the stream docs specifying that at most a single
call to _transform can happen, and the provided callback()
should be used to process another chunk.

Fixes: #3208
PR-URL: #14321
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
Add a note to the stream docs specifying that at most a single
call to _transform can happen, and the provided callback()
should be used to process another chunk.

Fixes: #3208
PR-URL: #14321
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Sep 3, 2017
Add a note to the stream docs specifying that at most a single
call to _transform can happen, and the provided callback()
should be used to process another chunk.

Fixes: #3208
PR-URL: #14321
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Sep 5, 2017
Add a note to the stream docs specifying that at most a single
call to _transform can happen, and the provided callback()
should be used to process another chunk.

Fixes: #3208
PR-URL: #14321
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants