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

Jump to bottom

x/crypto/ssh/agent: TestServerResponseTooLarge fails on Plan 9 #35888

Closed
fhs opened this issue Nov 28, 2019 · 1 comment
Closed

x/crypto/ssh/agent: TestServerResponseTooLarge fails on Plan 9 #35888

fhs opened this issue Nov 28, 2019 · 1 comment
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Plan9 Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Unreleased

Comments

@fhs
Copy link
Contributor

fhs commented Nov 28, 2019

What version of Go are you using (go version)?

$ go version
go version devel +master Tue Nov 26 02:05:50 EST 2019 plan9/386

What did you do?

go test -v -run TestServerResponseTooLarge -count 2

What did you expect to see?

Tests pass

What did you see instead?

=== RUN   TestServerResponseTooLarge
--- PASS: TestServerResponseTooLarge (0.06s)
=== RUN   TestServerResponseTooLarge
panic: Log in goroutine after TestServerResponseTooLarge has completed

goroutine 11 [running]:
testing.(*common).logDepth(0x10c560a0, 0x10b280c0, 0x51, 0x3)
	/home/big/go/src/testing/testing.go:681 +0x384
testing.(*common).log(...)
	/home/big/go/src/testing/testing.go:663
testing.(*common).Fatalf(0x10c560a0, 0x20d03a, 0x52, 0x10c6dfb4, 0x1, 0x1)
	/home/big/go/src/testing/testing.go:724 +0x64
golang.org/x/crypto/ssh/agent.TestServerResponseTooLarge.func1(0x2479c0, 0x10818740, 0x1, 0x10c80000, 0x1000001, 0x1000001, 0x10c560a0)
	/home/fhs/go/src/golang.org/x/crypto/ssh/agent/client_test.go:332 +0x13f
created by golang.org/x/crypto/ssh/agent.TestServerResponseTooLarge
	/home/fhs/go/src/golang.org/x/crypto/ssh/agent/client_test.go:329 +0x1be
exit status: 'agent.test 185685: 2'
FAIL	golang.org/x/crypto/ssh/agent	0.230s

What's happening is that the test launches a goroutine but doesn't wait for it to finish, so the test passes. Later on, the launched goroutine calls t.Fatalf, so we get that panic. After modifying the test to give a better error:

cpu% go test -v -run TestServerResponseTooLarge
=== RUN   TestServerResponseTooLarge
    TestServerResponseTooLarge: client_test.go:342: At least 4 bytes (the response size) should have been successfully written: 0 < 4: write tcp 127.0.0.1:50054->127.0.0.1:46948: write /net/tcp/25/data: Hangup
--- FAIL: TestServerResponseTooLarge (0.05s)
FAIL
exit status: 'agent.test 186082: 1'
FAIL	golang.org/x/crypto/ssh/agent	0.186s

Sometimes the error is "i/o on hungup channel" instead of "Hangup".

This test is also failing on the builder:
https://build.golang.org/log/01ff8d9e8356be32a79e302c7db99bffdfb3b82a

@gopherbot add labels OS-Plan9, NeedsFix, Testing
@gopherbot gopherbot added this to the Unreleased milestone Nov 28, 2019
@gopherbot gopherbot added NeedsFix The path to resolution is known, but the work has not been done. OS-Plan9 Testing An issue that has been verified to require only test changes, not just a test failure. labels Nov 28, 2019
@gopherbot
Copy link

Change https://golang.org/cl/209298 mentions this issue: ssh/agent: fix TestServerResponseTooLarge on Plan 9

@golang golang locked and limited conversation to collaborators Nov 27, 2020
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@fhs @hanwen
ssh/agent: fix TestServerResponseTooLarge on Plan 9
a976ab3 
First, modify the test to report a better error by waiting for the
Marshal+Write goroutine to finish before returning from the test. If we
return too early, a failure inside that goroutine can generate a panic.

Second, we workaround plan9 not returning the actual number of bytes
written on the connection in case of a hangup (due to closed
connection). I've verified that syscall.Pwrite returns -1 on hangup in
this particular case even when some data did get written.

Fixes golang/go#35888

Change-Id: I7998cff926295f0d577b125c137021a9adc1be5a
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/209298
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@fhs @hanwen
ssh/agent: fix TestServerResponseTooLarge on Plan 9
d475a21 
First, modify the test to report a better error by waiting for the
Marshal+Write goroutine to finish before returning from the test. If we
return too early, a failure inside that goroutine can generate a panic.

Second, we workaround plan9 not returning the actual number of bytes
written on the connection in case of a hangup (due to closed
connection). I've verified that syscall.Pwrite returns -1 on hangup in
this particular case even when some data did get written.

Fixes golang/go#35888

Change-Id: I7998cff926295f0d577b125c137021a9adc1be5a
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/209298
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@fhs @hanwen
ssh/agent: fix TestServerResponseTooLarge on Plan 9
179442f 
First, modify the test to report a better error by waiting for the
Marshal+Write goroutine to finish before returning from the test. If we
return too early, a failure inside that goroutine can generate a panic.

Second, we workaround plan9 not returning the actual number of bytes
written on the connection in case of a hangup (due to closed
connection). I've verified that syscall.Pwrite returns -1 on hangup in
this particular case even when some data did get written.

Fixes golang/go#35888

Change-Id: I7998cff926295f0d577b125c137021a9adc1be5a
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/209298
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@fhs @hanwen
ssh/agent: fix TestServerResponseTooLarge on Plan 9
08d7c48 
First, modify the test to report a better error by waiting for the
Marshal+Write goroutine to finish before returning from the test. If we
return too early, a failure inside that goroutine can generate a panic.

Second, we workaround plan9 not returning the actual number of bytes
written on the connection in case of a hangup (due to closed
connection). I've verified that syscall.Pwrite returns -1 on hangup in
this particular case even when some data did get written.

Fixes golang/go#35888

Change-Id: I7998cff926295f0d577b125c137021a9adc1be5a
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/209298
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@fhs @hanwen
ssh/agent: fix TestServerResponseTooLarge on Plan 9
b830508 
First, modify the test to report a better error by waiting for the
Marshal+Write goroutine to finish before returning from the test. If we
return too early, a failure inside that goroutine can generate a panic.

Second, we workaround plan9 not returning the actual number of bytes
written on the connection in case of a hangup (due to closed
connection). I've verified that syscall.Pwrite returns -1 on hangup in
this particular case even when some data did get written.

Fixes golang/go#35888

Change-Id: I7998cff926295f0d577b125c137021a9adc1be5a
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/209298
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Plan9 Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
2 participants
@fhs @gopherbot