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/http: should stop logging received GOAWAY #42142

Open
jameshartig opened this issue Oct 22, 2020 · 2 comments
Open

net/http: should stop logging received GOAWAY #42142

jameshartig opened this issue Oct 22, 2020 · 2 comments
Milestone

Comments

@jameshartig
Copy link
Contributor

@jameshartig jameshartig commented Oct 22, 2020

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

$ go version 1.15.3

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

N/A

What did you do?

We see a lot of logging:

http2: received GOAWAY [FrameHeader GOAWAY len=20], starting graceful shutdown

without any indication of what to do or how to fix that. It seems like @bradfitz said we should just remove this log #18776 (comment) but it got lost in removing the other logs.

If we want to keep this log around, we should at least fix the message to include the ErrCode since just printing the frame header is useless.

What did you expect to see?

I expect to not see these logs if there's nothing the application can do to prevent them or if there's something actionable to do, then I expect to see the relevant error code to decide what to do.

What did you see instead?

Instead, I see these log messages over 7,000 times an hour.

@cagedmantis cagedmantis changed the title http: Stop logging received GOAWAY net/http: should stop logging received GOAWAY Oct 23, 2020
@cagedmantis cagedmantis added this to the Backlog milestone Oct 26, 2020
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Oct 26, 2020

@empijei
Copy link
Contributor

@empijei empijei commented Oct 27, 2020

Sounds like a bug to me, and indeed the linked discussion seems to suggest we should have removed those long time ago.

Looking at the code there seem to be already some logic to conditionally log these:

go/src/net/http/h2_bundle.go

Lines 5290 to 5297 in 1095dd6

func (sc *http2serverConn) processGoAway(f *http2GoAwayFrame) error {
sc.serveG.check()
if f.ErrCode != http2ErrCodeNo {
sc.logf("http2: received GOAWAY %+v, starting graceful shutdown", f)
} else {
sc.vlogf("http2: received GOAWAY %+v, starting graceful shutdown", f)
}
sc.startGracefulShutdownInternal()

So we should either change the first message to include the error or just default to call vlogf.

Since this doesn't seem like a particularly useful message unless stuff is being run in debug mode I would be in favor of the latter.

I don't have a strong opinion on either solutions but I'd be happy to prepare a CL to do either.

@jameshartig @bradfitz WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.