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

x/build/internal/https: buggy synchronization in ListenAndServe #29941

Open
bcmills opened this Issue Jan 25, 2019 · 5 comments

Comments

Projects
None yet
3 participants
@bcmills
Copy link
Member

bcmills commented Jan 25, 2019

This was going to be a drive-by comment on https://golang.org/cl/159698, but it's a separate (existing) bug and can be handled separately.

In this code:
https://github.com/golang/build/blob/ae7c9cf771eea29f108a62746cea3928b5759a2d/internal/https/https.go#L51-L64

If opt.AutocertCacheBucket != "", then as far as I can tell we end up starting two goroutines (http.Serve and serveAutocertTLS) using the same handler. We only wait for one of them to return, so the other goroutine ends up leaking and its error is ignored.

I suspect that we should only be making one of those two calls in the first place, in which case the need for the goroutine at all is very unclear.

(CC @dmitshur @bradfitz @andybons)

@gopherbot gopherbot added this to the Unreleased milestone Jan 25, 2019

@gopherbot gopherbot added the Builders label Jan 25, 2019

@dmitshur dmitshur self-assigned this Jan 25, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Jan 25, 2019

If opt.AutocertCacheBucket != "", then as far as I can tell we end up starting two goroutines (http.Serve and serveAutocertTLS) using the same handler.

It does start two goroutines in that case, but they don't use the same handler. The HTTPS goroutine uses the handler parameter that was passed in, but the HTTP goroutine uses the http.HandlerFunc(redirectToHTTPS) handler.

We only wait for one of them to return, so the other goroutine ends up leaking and its error is ignored.

This is true. However, in the most common use of ListenAndServe, which looks like log.Fatalln(https.ListenAndServe(...)), as soon as either of the two http.Serve and serveAutocertTLS returns an error, the entire program exits.

This also relies on the important property that both http.Serve and serveAutocertTLS only return non-nil errors. If they could return nil errors, then https.ListenAndServe could return a nil error from the first and not see a non-nil error from the second goroutine.

I suspect that we should only be making one of those two calls in the first place, in which case the need for the goroutine at all is very unclear.

As I understand, at least one goroutine is required to start an HTTPS and HTTP server at once. Two goroutines are used simply for symmetry, so both can write their error into the same errc channel.

We could improve this, but the value is low. The x/build/internal/https package has only one user at this time: x/build/cmd/gerritbot.

I can send a CL that fixes the goroutine leak and make the code more readable, but it'll still suffer from the property that when ListenAndServe returns due to one of the two servers (HTTPS or HTTP) returning an error, the other server isn't closed (again, because it's expected the command to consider an error from ListenAndServe to be fatal and exit). Fixing that is more invasive and probably not worth it at this time.

In general, I prefer this code to live inside the command that's starting up the HTTP(S) servers, then it's easier to customize it for the exact needs of the program and not leak resources.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jan 25, 2019

Change https://golang.org/cl/159700 mentions this issue: internal/https: refactor ListenAndServe code

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jan 25, 2019

Change https://golang.org/cl/159701 mentions this issue: x/build/internal/https: wait for both handlers in ListenAndServe

@bcmills

This comment has been minimized.

Copy link
Member Author

bcmills commented Jan 25, 2019

as soon as either of the two http.Serve and serveAutocertTLS returns an error, the entire program exits.

I would really rather we minimize cross-package reasoning like that: it makes package invariants much more difficult to see and maintain. Even in internal packages, folks outside the Go project look to our code for examples, so our code should be exemplary.

@bcmills

This comment has been minimized.

Copy link
Member Author

bcmills commented Jan 25, 2019

Fixing that is more invasive and probably not worth it at this time.

Doesn't seem terribly invasive to me: see https://golang.org/cl/159701.

@dmitshur dmitshur removed their assignment Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment