Repository navigation
IBM i test failures (read ECONNRESET) #39683
Description
Activity
FWIW https://ci.nodejs.org/job/node-test-commit-ibmi/469/nodes=ibmi73-ppc64/#showFailuresLink shows the same seven tests failing on test-iinthecloud-ibmi73-ppc64_be-2.
@richardlau see #39525 (comment). Can you try to rerun the tests with #36111 applied?
@richardlau see #39525 (comment). Can you try to rerun the tests with #36111 applied?
https://ci.nodejs.org/job/node-test-commit-ibmi/471/nodes=ibmi73-ppc64/
https://ci.nodejs.org/job/node-test-commit-ibmi/471/nodes=ibmi73-ppc64/ was #36111 rebased on top of 822f9ff.
Details
https://ci.nodejs.org/job/node-test-commit-ibmi/471/nodes=ibmi73-ppc64/consoleFull
02:33:29 HEAD detached at ebea6f5369 02:33:29 nothing to commit, working tree clean 02:33:29 ++ git rev-parse HEAD 02:33:29 ebea6f5369e056169f545713e4cc8b03ec402755 02:33:29 ++ git rev-parse origin/master 02:33:29 822f9ff4e6f819cfcf37c043bfddb441197f0723 02:33:29 ++ '[' -n origin/master ']' 02:33:29 ++ git rebase --committer-date-is-author-date origin/master 02:33:33 First, rewinding head to replay your work on top of it... 02:33:36 Applying: TLS: improve handling of shutdown 02:33:36 Applying: Apply suggestions from code reviewReacted by Luigi Pinca@richardlau I think reverting libuv/libuv#3006 might fix the problem as the error is coming from line 220 here and this has something to do with
EOF:
node/lib/internal/stream_base_commons.js
Lines 217 to 222 in 52ebe0f
libuv/libuv#3006 may be likely to exacerbate existing race conditions in nodejs (since it surfaces attempted writes-after-shutdown as errors somewhat quicker), but it seems unlikely to cause them.
Looking at parallel/test-http-client-parse-error, this test server appears to violate the TCP protocol (valid, as that is the point of the test), which is causing the client to crash (invalid, as it means the test is correctly determining that nodejs is broken here). This test defines a TCP server which does not read any of the incoming data, which means the TCP spec requires that the kernel send a ECONNRESET packet (exactly what we sometimes see happen there). However, that server also immediately writes data then calls
shutdown. Thus, depending on which side (client or server) can buffer their data faster and move that data over the localhost network, we expect that the test should sometimes pass (if the server sees TCP FIN first) and sometimes fail (if the client sees ECONNRESET first), until the underlying nodejs bug is fixed.The nodejs failure to handle this situation correctly in the client seems related to be #39363, though in the http stack instead of the TLS stack. My fix #36111 is somewhat related, but only for the happy path, and doesn't fix the error path in nodejs (which is what we see triggering failures in the tests above).
- added 2 commits that reference this issue
on Feb 1, 2022 - added a commit that references this issue
on Feb 8, 2022 - added a commit that references this issue
on Feb 15, 2022 - added a commit that references this issue
on Feb 15, 2022 21 remaining items
The description for MF69723 looks good to me as a fix for this issue (ECONNRESET should not be set for close). MF69703 sounds to me like a new violation of the TCP standard (RST packets are supposed to destroy the read queue, superseding and discarding any waiting data).
I have received some feedback and clarification to the above from the team that worked on both PTFs.
According to them, the change made in MF69703 does not violate the TCP standard: The sockets receive code path was updated to return valid data queued on the socket, not the TCP receive queue. If there is pending data at the TCP layer when the RST is received, it is handled according to RFC 793.
I believe that RFC 793 specifies that all segment queues should be flushed and any outstanding RECEIVES should report a reset error instead:
If the RST bit is set then, any outstanding RECEIVEs and SEND should receive "reset" responses. All segment queues should be flushed. Users should also receive an unsolicited general "connection reset" signal. Enter the CLOSED state, delete the TCB, and return.After MF69703, the code is now not deleting the TCB nor flushing the incoming queue, and is continuing to process outstanding RECIEVEs. Either way though, the client application should eventually receive the error once the queue is drained, so it is not a significant issue, but it might be different than how I interpret other platforms would handle this.
- added a commit that references this issue
on Aug 14, 2022 - added a commit that references this issue
on Aug 16, 2022 - added a commit that references this issue
on Aug 23, 2022 - added a commit that references this issue
on Sep 5, 2022 - added 2 commits that reference this issue
on Jan 3, 2023 This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.@abmusse is this still an issue?
Starting with yesterday's daily build, node-test-commit-ibmi is seeing several test failures with
Error: read ECONNRESET).Earlier builds (e.g. https://ci.nodejs.org/job/node-test-commit-ibmi/466/) are passing -- the source change between the last passing build and the failing builds is c61870c (the libuv 1.42.0 update). For the record, the libuv builds (libuv-test-commit-ibmi and libuv-test-commit-ibmi-cmake) have been passing.
Both the failing builds were on test-iinthecloud-ibmi73-ppc64_be-1. I've started a new build (in progress) on test-iinthecloud-ibmi73-ppc64_be-2 to check whether it's host specific: https://ci.nodejs.org/job/node-test-commit-ibmi/469/nodes=ibmi73-ppc64/
cc @nodejs/platform-ibmi