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

We cannot avoid continuing to call SetReadDeadline and similar methods after the connection is closed. #1835

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

newacorn
Copy link
Contributor

In the current implementation, there are several places where it's assumed that SetReadDeadline and similar functions logically won't affect a closed connection. However, these assumptions may not hold in certain specific situations. Therefore, instead of asserting that such errors will never occur, simply return the errors encountered during the execution of these methods.

The current implementation of ShutDown can, under certain conditions, invalidate these assumptions.

Case 1

fasthttp/server.go

Lines 1600 to 1609 in d29a2b9

if err := c.SetReadDeadline(time.Now().Add(s.ReadTimeout)); err != nil {
panic(fmt.Sprintf("BUG: error in SetReadDeadline(%v): %v", s.ReadTimeout, err))
}
}
if s.WriteTimeout > 0 {
if err := c.SetWriteDeadline(time.Now().Add(s.WriteTimeout)); err != nil {
panic(fmt.Sprintf("BUG: error in SetWriteDeadline(%v): %v", s.WriteTimeout, err))
}
}

We cannot assume that when the program reaches this point, the connection state has not yet transitioned from StateNew to the actual StateIdle. Even if this transition takes 5 seconds, we cannot make assumptions about the scheduler without synchronization primitives.

Case 2

fasthttp/server.go

Lines 2136 to 2146 in d29a2b9

if handler, ok := s.nextProtos[proto]; ok {
// Remove read or write deadlines that might have previously been set.
// The next handler is responsible for setting its own deadlines.
if s.ReadTimeout > 0 || s.WriteTimeout > 0 {
if err := c.SetDeadline(zeroTime); err != nil {
panic(fmt.Sprintf("BUG: error in SetDeadline(zeroTime): %v", err))
}
}
return handler(c)
}

What ensures that getNextProto doesn't take longer than 5 seconds to execute?

Case 3

fasthttp/server.go

Lines 2262 to 2267 in d29a2b9

if reqConf.ReadTimeout > 0 {
deadline := time.Now().Add(reqConf.ReadTimeout)
if err := c.SetReadDeadline(deadline); err != nil {
panic(fmt.Sprintf("BUG: error in SetReadDeadline(%v): %v", deadline, err))
}
}

If neither Server.ReadTimeout nor Server.IdleTimeout are set, and the timeout is configured through Server.HeaderReceived, there is a possibility that the connection might be closed by ShutDown during the transition from idle to active state.
See Issue: #1815

Case 4

fasthttp/server.go

Lines 2403 to 2414 in d29a2b9

if writeTimeout > 0 {
if err := c.SetWriteDeadline(time.Now().Add(writeTimeout)); err != nil {
panic(fmt.Sprintf("BUG: error in SetWriteDeadline(%v): %v", writeTimeout, err))
}
previousWriteTimeout = writeTimeout
} else if previousWriteTimeout > 0 {
// We don't want a write timeout but we previously set one, remove it.
if err := c.SetWriteDeadline(zeroTime); err != nil {
panic(fmt.Sprintf("BUG: error in SetWriteDeadline(zeroTime): %v", err))
}
previousWriteTimeout = 0
}

If there's buffered data in the underlying connection or in br, and bw.Flush hasn't been executed before, it's not guaranteed that the underlying connection's closure will be observed by the code executed previously.

…s after the connection is closed.

In the current implementation, there are several places where it's assumed that SetReadDeadline and similar functions logically won't affect a closed connection. However, these assumptions may not hold in certain specific situations. Therefore, instead of asserting that such errors will never occur, simply return the errors encountered during the execution of these methods.
@erikdubbelboer erikdubbelboer merged commit 43c7b83 into valyala:master Aug 21, 2024
19 checks passed
@erikdubbelboer
Copy link
Collaborator

Makes sense, thanks for the change!

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

Successfully merging this pull request may close these issues.

2 participants