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.

Sign up for GitHub

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

Jump to bottom

doc: clarify when writable.end callback is called #4810

Closed
wants to merge 1 commit into from
Closed

doc: clarify when writable.end callback is called #4810

wants to merge 1 commit into from

Conversation

kevinoid
Copy link
Contributor

The current documentation for writable.end only specifies that the callback is called "once the data has been fully handled". It is ambiguous whether this means "successfully handled" and, if so, whether the callback is called if the data can not be successfully handled (i.e. an error occurred).

The ambiguity is not only in the documentation. The stream class implementations differ on this point. stream.Writable invokes the callback with any errors that occur during parameter checking or during calls to _write. However, not all classes return all errors to _write. zlib.Zlib does pass argument and state errors to the _write (_transform) callback, but does not pass data errors. http.OutgoingMessage passes argument type errors and some other types of errors, but not all.

This inconsistency is behind issue #1746 and, I suspect, other issues in client code which passes a callback to write.

This commit takes no position on whether the callback error behavior should changed, but simply attempts to document the current behavior in a way that is open to changes so that users are not caught by surprise.

Thanks for considering,
Kevin

@kevinoid
doc: clarify when writable.end callback is called
e5c71a7 
The current documentation for writable.end only specifies that the
callback is called "once the data has been fully handled".  It is
ambiguous whether this means "successfully handled" and, if so, whether
the callback is called if the data can not be successfully handled (i.e.
an error occurs).

The ambiguity is not only in the documentation.  The stream class
implementations differ on this point.  stream.Writable invokes the
callback with any errors that occur during parameter checking or during
calls to _write.  However, not all classes return all errors to _write.
zlib.Zlib does pass argument and state errors to the _write (_transform)
callback, but does not pass data errors.  http.OutgoingMessage passes
argument type errors and some other types of errors, but not all.

This inconsistency is behind issue nodejs#1746 and, I suspect, other issues in
client code which passes a callback to write.

This commit takes no position on whether the callback error behavior
should changed, but simply attempts to document the current behavior in
a way that is open to changes so that users are not caught by surprise.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
@mscdex mscdex added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Jan 22, 2016
@jasnell
Copy link
Member

jasnell commented Jan 22, 2016

Sigh.. "may or may not" ...we should fix that
LGTM!
Thank you!

@kevinoid
Copy link
Contributor Author

Indeed. If there's agreement that it's worth fixing, I volunteer to send PRs for zlib, since I've been looking at that code closely and there is some low-hanging fruit.

Reliably calling the callback from the http streams looks more painful. There are some low-hanging ones, but some will be pretty complicated.

@kevinoid
Copy link
Contributor Author

I just realized the commit message and PR refer to writable.end, but the commit fixes writable.write. I will fix that, but when I do it may be useful to update the writable.end docs as well. Although they are correct, a casual reader may miss the fact that the callback is not called on errors, which is an unusual convention. Thoughts?

@benjamingr
Copy link
Member

LGTM
I'd rather merge, this has been open for a while (2 months) - I can edit your commit message - please feel free to create another pull request for the docs of writable.end.

Thanks for the contribution - any objection I merge this as is?

@kevinoid
Copy link
Contributor Author

Sounds good to me. No objection to merging it as-is. Thanks @benjamingr!

@whitlockjc
Copy link
Contributor

LGTM

benjamingr pushed a commit that referenced this pull request Mar 14, 2016
@kevinoid @benjamingr
doc: clarify when writable.write callback is called
d955243 
The current documentation for writable.write only specifies that the
callback is called "once the data has been fully handled".  It is
ambiguous whether this means "successfully handled" and, if so,
whether the callback is called if the data can not be successfully
handled (i.e. an error occurs).

The ambiguity is not only in the documentation.  The stream class
implementations differ on this point.  stream.Writable invokes the
callback with any errors that occur during parameter checking or
during calls to _write.  However, not all classes return all errors
to _write. zlib.Zlib does pass argument and state errors to the
_write (_transform) callback, but does not pass data errors.
http.OutgoingMessage passes argument type errors and some other types
of errors, but not all.

This inconsistency is behind issue #1746 and, I suspect, other issues
in client code which passes a callback to write.

This commit takes no position on whether the callback error behavior
should changed, but simply attempts to document the current behavior
in a way that is open to changes so that users are not caught by
surprise.

PR-URL: #4810
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremy Whitlock <jcscoobyrs@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@benjamingr
Copy link
Member

Thanks, landed in d955243

@benjamingr benjamingr closed this Mar 14, 2016
@benjamingr
Copy link
Member

Tips for next time:

  • we have a PR-URL: field that links to the PR and Reviewed- By for reviewers.
  • commit messages are trimmed at 70 chars.
  • we rebase on top of master before merging.

I did that for you (as well as the small text fix), so don't worry about it. You don't have to worry about it too much in the future too - just for reference.

Thanks for the contribution!

@whitlockjc
Copy link
Contributor

Minor nit @benjamingr. We're within the 10 minute window to update the message but the email for me should be jwhitlock@apache.org and not jcscoobyrs@gmail.com. I know how you came up with that, my GitHub profile, and that's been fixed.

If you don't want to fix it, not a big deal.

@benjamingr
Copy link
Member

A paste typo most likely, fixed real quick, thanks for noticing :)

benjamingr pushed a commit that referenced this pull request Mar 14, 2016
@kevinoid @benjamingr
doc: clarify when writable.write callback is called
b801e39 
The current documentation for writable.write only specifies that the
callback is called "once the data has been fully handled".  It is
ambiguous whether this means "successfully handled" and, if so,
whether the callback is called if the data can not be successfully
handled (i.e. an error occurs).

The ambiguity is not only in the documentation.  The stream class
implementations differ on this point.  stream.Writable invokes the
callback with any errors that occur during parameter checking or
during calls to _write.  However, not all classes return all errors
to _write. zlib.Zlib does pass argument and state errors to the
_write (_transform) callback, but does not pass data errors.
http.OutgoingMessage passes argument type errors and some other types
of errors, but not all.

This inconsistency is behind issue #1746 and, I suspect, other issues
in client code which passes a callback to write.

This commit takes no position on whether the callback error behavior
should changed, but simply attempts to document the current behavior
in a way that is open to changes so that users are not caught by
surprise.

PR-URL: #4810
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@kevinoid
Copy link
Contributor Author

Great. Thanks for fixing it up and merging it! (And the preceding reviews!)

I'll omit the Signed-off-by and limit commit messages to 70 chars wide in the future, thanks for the heads-up! (I promise it was based on master when submitted, I'll continue to do that.)

evanlucas pushed a commit that referenced this pull request Mar 14, 2016
@kevinoid @evanlucas
doc: clarify when writable.write callback is called
be55939 
The current documentation for writable.write only specifies that the
callback is called "once the data has been fully handled".  It is
ambiguous whether this means "successfully handled" and, if so,
whether the callback is called if the data can not be successfully
handled (i.e. an error occurs).

The ambiguity is not only in the documentation.  The stream class
implementations differ on this point.  stream.Writable invokes the
callback with any errors that occur during parameter checking or
during calls to _write.  However, not all classes return all errors
to _write. zlib.Zlib does pass argument and state errors to the
_write (_transform) callback, but does not pass data errors.
http.OutgoingMessage passes argument type errors and some other types
of errors, but not all.

This inconsistency is behind issue #1746 and, I suspect, other issues
in client code which passes a callback to write.

This commit takes no position on whether the callback error behavior
should changed, but simply attempts to document the current behavior
in a way that is open to changes so that users are not caught by
surprise.

PR-URL: #4810
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@evanlucas evanlucas mentioned this pull request Mar 14, 2016
Merged
rvagg pushed a commit that referenced this pull request Mar 16, 2016
@kevinoid @rvagg
doc: clarify when writable.write callback is called
eb73574 
The current documentation for writable.write only specifies that the
callback is called "once the data has been fully handled".  It is
ambiguous whether this means "successfully handled" and, if so,
whether the callback is called if the data can not be successfully
handled (i.e. an error occurs).

The ambiguity is not only in the documentation.  The stream class
implementations differ on this point.  stream.Writable invokes the
callback with any errors that occur during parameter checking or
during calls to _write.  However, not all classes return all errors
to _write. zlib.Zlib does pass argument and state errors to the
_write (_transform) callback, but does not pass data errors.
http.OutgoingMessage passes argument type errors and some other types
of errors, but not all.

This inconsistency is behind issue #1746 and, I suspect, other issues
in client code which passes a callback to write.

This commit takes no position on whether the callback error behavior
should changed, but simply attempts to document the current behavior
in a way that is open to changes so that users are not caught by
surprise.

PR-URL: #4810
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
@kevinoid
doc: clarify when writable.write callback is called
fd268fd 
The current documentation for writable.write only specifies that the
callback is called "once the data has been fully handled".  It is
ambiguous whether this means "successfully handled" and, if so,
whether the callback is called if the data can not be successfully
handled (i.e. an error occurs).

The ambiguity is not only in the documentation.  The stream class
implementations differ on this point.  stream.Writable invokes the
callback with any errors that occur during parameter checking or
during calls to _write.  However, not all classes return all errors
to _write. zlib.Zlib does pass argument and state errors to the
_write (_transform) callback, but does not pass data errors.
http.OutgoingMessage passes argument type errors and some other types
of errors, but not all.

This inconsistency is behind issue #1746 and, I suspect, other issues
in client code which passes a callback to write.

This commit takes no position on whether the callback error behavior
should changed, but simply attempts to document the current behavior
in a way that is open to changes so that users are not caught by
surprise.

PR-URL: #4810
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
@kevinoid
doc: clarify when writable.write callback is called
0830bb4 
The current documentation for writable.write only specifies that the
callback is called "once the data has been fully handled".  It is
ambiguous whether this means "successfully handled" and, if so,
whether the callback is called if the data can not be successfully
handled (i.e. an error occurs).

The ambiguity is not only in the documentation.  The stream class
implementations differ on this point.  stream.Writable invokes the
callback with any errors that occur during parameter checking or
during calls to _write.  However, not all classes return all errors
to _write. zlib.Zlib does pass argument and state errors to the
_write (_transform) callback, but does not pass data errors.
http.OutgoingMessage passes argument type errors and some other types
of errors, but not all.

This inconsistency is behind issue #1746 and, I suspect, other issues
in client code which passes a callback to write.

This commit takes no position on whether the callback error behavior
should changed, but simply attempts to document the current behavior
in a way that is open to changes so that users are not caught by
surprise.

PR-URL: #4810
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kevinoid @jasnell @benjamingr @whitlockjc @mscdex @MylesBorins