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

x/net/http2: window updates on randomWriteScheduler after stream closed cause memory leaks [1.13 backport] #34636

Closed
gopherbot opened this issue Oct 1, 2019 · 4 comments

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 1, 2019

@bradfitz requested issue #33812 to be considered for backport to the next 1.13 minor release.

@jared2501, thanks for the great bug report & diagnosis!

@gopherbot, please backport to Go 1.13.

/cc @tombergan

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 3, 2019

@andybons, @bcmills, okay for backport?

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Oct 3, 2019

Change https://golang.org/cl/198617 mentions this issue: [release-branch.go1.13] http2: fix memory leak in random write scheduler

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Oct 4, 2019

Closed by merging 7ce5fdcd922f5ff36c5692af8042450629e41b3c to release-branch.go1.13.

@gopherbot gopherbot closed this Oct 4, 2019
gopherbot pushed a commit to golang/net that referenced this issue Oct 4, 2019
In certain shutdown cases (from the client and/or server), the http2
Server can Push stream-specific frames on closed streams. This caused
memory leaks in the random write scheduler.

As a conservative fix for backporting, just clear the map element
whenever its queue value is empty. The map entry is re-created as
needed anyway. This isn't perfectly ideal (it adds a map+delete and
free queue put+get) in the case where a stream is open & actively
writing, but it's an easy fix for now. A future CL can optimize all
this code. It looks like there are some other good optimization
opportunities in related code anyway. But I'd rather that happen on
master and not be done in a backported change.

Updates golang/go#34636 (needs bundle to std before fixed)
Updates golang/go#33812

Change-Id: I21508ba2ebc361e8b8532d0d1cebf882e82c473c
Reviewed-on: https://go-review.googlesource.com/c/net/+/198462
Reviewed-by: Bryan C. Mills <bcmills@google.com>
(cherry picked from commit d98b1b4)
Reviewed-on: https://go-review.googlesource.com/c/net/+/198617
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Run-TryBot: Andrew Bonventre <andybons@golang.org>
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Oct 4, 2019

Change https://golang.org/cl/198897 mentions this issue: [release-branch.go1.13] net/http: update bundled http2 for memory leak fix

gopherbot pushed a commit that referenced this issue Oct 4, 2019
…k fix

This re-runs go generate with x/net checked out at CL 198617 on the
release-branch.go1.13 branch for:

   [release-branch.go1.13] http2: fix memory leak in random write scheduler

Fixes #34636
Updates #33812

Change-Id: Ibce630c6c7ffe43ff760d2ad40b44731c07ba870
Reviewed-on: https://go-review.googlesource.com/c/go/+/198897
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@katiehockman katiehockman modified the milestones: Go1.13.2, Go1.13.3 Oct 17, 2019
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
4 participants
You can’t perform that action at this time.