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.

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

net: only js/wasm returns an error on writes to closed connections #28650

Open
FiloSottile opened this issue Nov 8, 2018 · 4 comments
Open
Labels
arch-wasm WebAssembly issues NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-JS
Milestone

Comments

@FiloSottile
Copy link
Contributor

If a localhost connection is closed by the peer, all other GOOS seem to allow writes to happen without error, while js/wasm returns write tcp 127.0.0.1:XXX->127.0.0.1:1: Socket is not connected with XXX changing.

I admit it does feel like something that should return an error, and I don't know if there is a precise requirement to do otherwise, but js being the only GOOS to do it is very inconsistent, and not even NaCl on the playground with its fictitious network returns an error.

This came up because js/wasm were the only broken TryBots in some crypto/tls CL: https://storage.googleapis.com/go-build-log/ca02321c/js-wasm_d63aa3d8.log

https://play.golang.org/p/lRhq44NJXTr (this specific program will actually panic on js/wasm due to #28649, but it proves the log.Print is being reached)

/cc @neelance

@FiloSottile FiloSottile added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. arch-wasm WebAssembly issues labels Nov 8, 2018
@FiloSottile FiloSottile added this to the Go1.12 milestone Nov 8, 2018
@mikioh
Copy link
Contributor

mikioh commented Nov 8, 2018

Unfortunately, the fake implementation of TCP-like byte-sequence full-duplex pipe on JS is broken from the beginning. Also, it doubles the maintenance cost by having each implementation on NaCl and JS. I have a CL that may solve the issues: https://go-review.googlesource.com/c/go/+/120958, but it will probably land in Go 1.13 or above.

Can you please skip the roadblock test cases for now?

@FiloSottile
Copy link
Contributor Author

Yeah I adapted the tests, so not urgent on my side. Feel free to re-milestone as you wish.

What is blocking that CL?

@gopherbot
Copy link

Change https://golang.org/cl/147445 mentions this issue: crypto/tls: implement TLS 1.3 PSK authentication (server side)

@mikioh
Copy link
Contributor

mikioh commented Nov 8, 2018

What is blocking that CL?

Just for congestion control; the plan for Go 1.12 had AIX and Fuchsia ports that need to churn up the runtime, os and net packages. Like other communication protocols, congestion control is reviewee/sender side's responsibility (and flow control is reviewer/receiver side's responsibility.)

@mikioh mikioh modified the milestones: Go1.12, Unplanned Nov 8, 2018
@mikioh mikioh added OS-JS OS-NaCl GOOS=nacl, Native Client, removed in Go 1.14 and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. arch-wasm WebAssembly issues labels Nov 8, 2018
@FiloSottile FiloSottile added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 8, 2018
gopherbot pushed a commit that referenced this issue Nov 12, 2018
Added some assertions to testHandshake, but avoided checking the error
of one of the Close() because the one that would lose the race would
write the closeNotify to a connection closed on the other side which is
broken on js/wasm (#28650). Moved that Close() after the chan sync to
ensure it happens second.

Accepting a ticket with client certificates when NoClientCert is
configured is probably not a problem, and we could hide them to avoid
confusing the application, but the current behavior is to skip the
ticket, and I'd rather keep behavior changes to a minimum.

Updates #9671

Change-Id: I93b56e44ddfe3d48c2bef52c83285ba2f46f297a
Reviewed-on: https://go-review.googlesource.com/c/147445
Reviewed-by: Adam Langley <agl@golang.org>
@bradfitz bradfitz removed the OS-NaCl GOOS=nacl, Native Client, removed in Go 1.14 label Oct 8, 2019
@dmitshur dmitshur added the arch-wasm WebAssembly issues label Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly issues NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-JS
Projects
None yet
Development

No branches or pull requests

5 participants