-
Notifications
You must be signed in to change notification settings - Fork 20.2k
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
graphql: fix spurious travis failure #22166
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
graphql/graphql_test.go
Outdated
WSHost: "127.0.0.1", | ||
WSPort: 9393, | ||
WSPort: port, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use port zero here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Indeed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, PTAL
graphql/graphql_test.go
Outdated
}) | ||
if err != nil { | ||
t.Fatalf("could not create node: %v", err) | ||
} | ||
if !gqlEnabled { | ||
return stack | ||
} | ||
createGQLService(t, stack, fmt.Sprintf("127.0.0.1:%d", port)) | ||
createGQLService(t, stack, node.DefaultHTTPEndpoint()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bleh, I'll fix this
5c645ae
to
a0cf3cb
Compare
The tests sometimes failed with certain go versions because the behavior of http.Server.Shutdown changed over time. A bug that was fixed in Go 1.15 could cause active connections on unrelated servers to close unexpectedly. This is fixed by avoiding use of the same port number in all tests.
In go
1.14
, the graphql tests runs really fast:But sometimes they fail:
In go
1.15
, they run slow. ButBut they don't spuriously fail!
So the delay seems to be working. What is the delay? The delay is improved in this commit: golang/go@f0c9ae5 , and digging into the back-story a bit, it can be surmised that there's a race between closing connecttions, and that there may be active connections that get bit by it. It seems that
1.14
suffers from this. Maybe a straggling 'close' or 'reset' is still on the network when the next connection is made.Whatever the reason, it seems that a working fix is to not use the same port every time, but randomize it a bit. With this PR, I no longer trigger the error on go
1.14
.