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

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
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@jameshartig
Copy link
Contributor

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 26, 2020
@cagedmantis cagedmantis added this to the Backlog milestone Oct 26, 2020
@cagedmantis
Copy link
Contributor

/cc @bradfitz @empijei

@empijei
Copy link
Contributor

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
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@jameshartig @cagedmantis @empijei