server: cancel the request context on timeout #821
Conversation
ptrus
force-pushed
the
ptrus/feature/request-timeout-context
branch
3 times, most recently
from
c03e842 to
f22281d
Compare
| msg := err.Error() | ||
| errStruct := apiTypes.HumanReadableError{Msg: msg} | ||
| // If request context is closed, don't bother writing a response. | ||
| if r.Context().Err() != nil { |
There was a problem hiding this comment.
does this mean we leave the request hanging if our handling times out?
There was a problem hiding this comment.
If the context is canceled the client already received an EOF (due to the WriteTimeout being reached), so doing anything here will have no effect.
But maybe instead of using our own 'ContextTimeoutHandler' which only cancels the context, we should use upstream TimeoutHandler. That one also handles context cancellation, but it also returns a 503, instead of just closing the connection as it is done now. Thats probably nicer.
There was a problem hiding this comment.
Now that we use http.TimeoutHandler, the client receives a 503 and the connection is closed when the timeout is reached. So like before, if the context is canceled there is nothing to do. Even if we didn't return here, the Writes below would fail with ErrHandlerTimeout.
cmd/api/api.go
Outdated
| ReadTimeout: 10 * time.Second, | ||
| WriteTimeout: 10 * time.Second, | ||
| ReadTimeout: requestsTimeout, | ||
| WriteTimeout: requestsTimeout, |
There was a problem hiding this comment.
ReadTimeout and WriteTimeout are for request I/O and not handling right? these don't have to be the same as the requestsTimeout
There was a problem hiding this comment.
No, WriteTimeout is not just the IO for writing the response, it includes the server processing.
There was a problem hiding this comment.
oh, should that be some amount higher than the processing time limit then?
There was a problem hiding this comment.
Good idea, i think we should; and then by also using the upstream Timeout handler (i mentioned in the other comment) the client will get a 503 in case the processing timeout is reached.
ptrus
force-pushed
the
ptrus/feature/request-timeout-context
branch
2 times, most recently
from
97c031b to
f9c3bda
Compare
ptrus
force-pushed
the
ptrus/feature/request-timeout-context
branch
6 times, most recently
from
510a7b0 to
16cc9b5
Compare
ptrus
force-pushed
the
ptrus/feature/request-timeout-context
branch
from
16cc9b5 to
064658f
Compare
|
I'm merging since we want to do a new release today, can apply potential requestsed changes in a future PR. |
Added
ContextTimeoutMiddlewareto enforce request context timeout and ensure proper cancellation of timed-out requests. This ensures that the context for requests exceeding the defined timeout limit are properly canceled.I noticed cases like the one below, where request takes 15 seconds:
{"caller":"middleware.go:77","latency":"14.69731168s","latency_bin":">1000ms","level":"info","module":"api","msg":"ending request","query_params":"limit=10&offset=0","query_path":"/v1/sapphire/evm_tokens","request_id":"3fecdf6b-ad74-42f0-8cfc-52ba0787f05a","status_code":200,"ts":"2024-12-05T08:37:32.086384228Z"}Note that the server logs show a 200 OK status, but the response is not delivered to the client. Instead, the client receives an EOF since the configured server timeout is reached, while the server continues processing the request.
Ref upstream issue: golang/go#59602