valyala / fasthttp

Fast HTTP package for Go. Tuned for high performance. Zero memory allocations in hot paths. Up to 10x faster than net/http
MIT License
21.94k stars 1.76k forks source link

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

Closed newacorn closed 3 months ago

newacorn commented 3 months ago

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

https://github.com/valyala/fasthttp/blob/d29a2b90a63494f2007471d0653d5d85ead61397/server.go#L1600-L1609

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

https://github.com/valyala/fasthttp/blob/d29a2b90a63494f2007471d0653d5d85ead61397/server.go#L2136-L2146 What ensures that getNextProto doesn't take longer than 5 seconds to execute?

Case 3

https://github.com/valyala/fasthttp/blob/d29a2b90a63494f2007471d0653d5d85ead61397/server.go#L2262-L2267 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: https://github.com/valyala/fasthttp/issues/1815

Case 4

https://github.com/valyala/fasthttp/blob/d29a2b90a63494f2007471d0653d5d85ead61397/server.go#L2403-L2414 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.

erikdubbelboer commented 3 months ago

Makes sense, thanks for the change!