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

net/http: TestTransportPersistConnReadLoopEOF failures with Connection reset by peer on wasip1 #64317

Closed
bcmills opened this issue Nov 21, 2023 · 5 comments
Closed

net/http: TestTransportPersistConnReadLoopEOF failures with Connection reset by peer on wasip1 #64317

bcmills opened this issue Nov 21, 2023 · 5 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.23

Comments

@bcmills
Copy link
Contributor

bcmills commented Nov 21, 2023 •
edited 

#!watchflakes
post <- pkg == "net/http" && test == "TestTransportPersistConnReadLoopEOF" && `send: [Cc]onnection reset by peer`

Examples:

--- FAIL: TestTransportPersistConnReadLoopEOF (0.00s)
    transport_internal_test.go:62: pc.closed = http.nothingWrittenError{error:(*net.OpError)(0x181c5a0)}, write tcp 127.0.0.1:2->127.0.0.1:1: send: Connection reset by peer; want errServerClosedIdle or transportReadFromServerError
FAIL
FAIL	net/http	28.029s

See previously:

(CC @neild @golang/wasm)
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 21, 2023
@bcmills bcmills added this to the Backlog milestone Nov 21, 2023
@bcmills
Copy link
Contributor Author

bcmills commented Nov 21, 2023

Ah, I think this is related to #30938.

https://go.dev/cl/379554 changed the propagation of nothingWrittenError and also changed an earlier assertion in TestTransportPersistConnReadLoopEOF to allow it.

@bcmills bcmills added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsFix The path to resolution is known, but the work has not been done. labels Nov 21, 2023
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 21, 2023
@gopherbot

This comment was marked as resolved.

Sign in to view
@bcmills
Copy link
Contributor Author

bcmills commented Jan 25, 2024

Hmm. I wonder if we just need to crank up rstAvoidanceDelay for this test. (Search for SetRSTAvoidanceDelay and apply a similar loop, perhaps?)

@gopherbot
Copy link

Change https://go.dev/cl/558595 mentions this issue: net/http: ignore nothingWrittenError in TestTransportPersistConnReadLoopEOF

@gopherbot
Copy link

Change https://go.dev/cl/558915 mentions this issue: net: in the fake implementation, allow writes to buffer on closed connections

gopherbot pushed a commit that referenced this issue Jan 29, 2024
@prattmic @gopherbot
net/http: allow nothingWrittenError in TestTransportPersistConnReadLo…
3207f03 
…opEOF

Flakes in #64317 are a result of a race where the server shutdown
schedules ahead of the client read loop. Normal network latency usually
hides this, but wasm's net_fake.go has very low latency.

Explicitly allow the results of this race in the test.

For #64317.

Change-Id: I9c2572fb44643762fe3f3d7cb133d7e7a8a47881
Reviewed-on: https://go-review.googlesource.com/c/go/+/558595
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
@dmitshur dmitshur modified the milestones: Backlog, Go1.23 Jan 30, 2024
@prattmic prattmic mentioned this issue Jan 31, 2024
Closed
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@ezz-no
net: in the fake implementation, allow writes to buffer on closed con…
de7f63f 
…nections

This mimics the apparent behavior of writes on linux/amd64, in which a
write on an already-closed connection silently succeeds — even if the
connection has already been closed by the remote end — provided that
the packet fits in the kernel's send buffer.

I tested this by patching in CL 557437 and running the test on js/wasm
and wasip1/wasm locally.

Fixes golang#64317.

Change-Id: I43f6a89e5059115cb61e4ffc33a8371057cb67a1
Reviewed-on: https://go-review.googlesource.com/c/go/+/558915
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@prattmic @ezz-no
net/http: allow nothingWrittenError in TestTransportPersistConnReadLo…
4279efb 
…opEOF

Flakes in golang#64317 are a result of a race where the server shutdown
schedules ahead of the client read loop. Normal network latency usually
hides this, but wasm's net_fake.go has very low latency.

Explicitly allow the results of this race in the test.

For golang#64317.

Change-Id: I9c2572fb44643762fe3f3d7cb133d7e7a8a47881
Reviewed-on: https://go-review.googlesource.com/c/go/+/558595
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
Test Flakes
Status: Done
Milestone
Go1.23
Development

No branches or pull requests
3 participants
@dmitshur @bcmills @gopherbot