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.

Sign up for GitHub

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

Add timeout to writing to responses #15831

Merged
merged 4 commits into from
May 14, 2021
Merged

Add timeout to writing to responses #15831

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e181b3
Add timeout to writing to responses
zeripath May 11, 2021
46cf319
Merge branch 'main' into fix-15826-timeout-writing-to-responses
6543 May 13, 2021
5e3d79a
Merge branch 'main' into fix-15826-timeout-writing-to-responses
zeripath May 14, 2021
e33a3fd
Merge branch 'main' into fix-15826-timeout-writing-to-responses
zeripath May 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions modules/graceful/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ var (
DefaultMaxHeaderBytes int
)

// PerWriteWriteTimeout timeout for writes
const PerWriteWriteTimeout = 5 * time.Second
Copy link
Member

@noerw noerw May 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 secs seem a lot; if kernel blocks for that duration the system has bigger problems..
200ms seems to be a lower bound due to write batching with NAGLE (?), I'd go with 500ms?
But as a stopgap to avoid lingering connections a large value seems ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I picked 5s as value everyone could agree meant that the underlying connection is totally blocked and just wasn't coming back. But I have no metric to know what is a good value here as I don't completely understand the situations where this happens.

I'm suspecting that the problem is due to some network reconfiguration that leads to the connection at the go end being disconnected from the client but somehow didn't kill the connection.


func init() {
DefaultMaxHeaderBytes = 0 // use http.DefaultMaxHeaderBytes - which currently is 1 << 20 (1MB)
}
Expand Down Expand Up @@ -250,6 +253,13 @@ type wrappedConn struct {
closed *int32
}

func (w wrappedConn) Write(p []byte) (n int, err error) {
if PerWriteWriteTimeout > 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why check? It's a const value, isn't it?

Copy link
Contributor Author

@zeripath zeripath May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A const that is labelled so that you can change it at compile time to something else...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.

LDFLAGS="-X \"code.gitea.io/gitea/modules/graceful.PerWriteWriteTimeout=0\""

or

LDFLAGS="-X \"code.gitea.io/gitea/modules/graceful.PerWriteWriteTimeout=1s\""

_ = w.Conn.SetWriteDeadline(time.Now().Add(PerWriteWriteTimeout))
}
return w.Conn.Write(p)
}

func (w wrappedConn) Close() error {
if atomic.CompareAndSwapInt32(w.closed, 0, 1) {
defer func() {
Expand Down