| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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>
|
Sigh.. "may or may not" ...we should fix that |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
LGTM Thanks for the contribution - any objection I merge this as is? |
Sorry, something went wrong.
|
Sounds good to me. No objection to merging it as-is. Thanks @benjamingr! |
Sorry, something went wrong.
|
LGTM |
Sorry, something went wrong.
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>
|
Tips for next time:
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! |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
A paste typo most likely, fixed real quick, thanks for noticing :) |
Sorry, something went wrong.
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>
|
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.) |
Sorry, something went wrong.
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>
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>
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>
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>
| Back | FazBrowse Home | New Git URL |
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