Revert "stream: squelch ECONNRESET error if already closed" #475
Conversation
|
Rubber-stamp LGTM but can I suggest you mention in the commit log that it was causing regressions in the io.js test suite? /cc @santigimeno |
|
@bnoordhuis I added a "Refs" there, do you want me to elaborate further? |
|
Just that it's nice for people that work offline (or that are just lazy, like me, and don't want to copy/paste the link into a browser) if the commit log tells you why the revert was necessary (test failures.) |
|
Maybe we could just revert the Linux part ( |
saghul
force-pushed
the
econnreset_revert
branch
from
2471526 to
cc28fbb
Compare
|
@bnoordhuis updated. |
|
@santigimeno This patch caused other problems as well, see #474, I'd rather revert it in full now and reevaluate afterwards. |
|
Fair enough |
|
doesnt matter the system, closing a descriptor due to an error sounds inherently tricky |
|
@reqshark What do you mean? The patch didn't close any file descriptor due to an error, it replaced ECONNRESET with EOF under certain conditions. |
|
ECONNRESET means the other side closes the connection I think. If the system doesn't automatically close that descriptor shouldn't the programmer? |
|
The fd is closed when you call |
This reverts commit 05a003a. This commit triggerd "test-tls-hello-parser-failure" failure in io.js. See the reference below for a more thorough explanation. Refs: nodejs/node#2310 PR-URL: libuv#475 Reviewed-By: Ben Noordhuis <[email protected]>
saghul
force-pushed
the
econnreset_revert
branch
from
cc28fbb to
01544d8
Compare
|
@saghul I just mean to say it seems like a tricky scenario.. btw i looked here https://github.com/libuv/libuv/pull/475/files#diff-c9c9805d800eb132a191a9e40da6d813R1220 UV__POLLHUP could be set and ignored. If the connection gets closed why bother doing read? |
How would the user know otherwise? |
|
ah I see. makes sense to me. thank you :) |
|
Question: I thought the patch would make behavior on linux & OSX equal, but OSX passed just fine. What caused linux to break but not OSX? |
This reverts commit ca08b48, for the same reasons that it was reverted the last time it was merged. Refs: libuv#475 Refs: libuv#2447 PR-URL: libuv#2602 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
This reverts commit ca08b48, for the same reasons that it was reverted the last time it was merged. Refs: libuv#475 Refs: libuv#2447 PR-URL: libuv#2602 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
This reverts commit ca08b48, for the same reasons that it was reverted the last time it was merged. Refs: libuv/libuv#475 Refs: libuv/libuv#2447 PR-URL: libuv/libuv#2602 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
This reverts commit ca08b48, for the same reasons that it was reverted the last time it was merged. Refs: libuv/libuv#475 Refs: libuv/libuv#2447 PR-URL: libuv/libuv#2602 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
5 participants
This reverts commit 05a003a.
Refs: nodejs/node#2310 (comment)