update timeout middleware with warnings #218
Conversation
|
todo: probably should mention Nginx/Apache etc similar behavior and configuration settings |
| @@ -8,48 +8,62 @@ description = "Timeout middleware for Echo" | |||
|
|
|||
| Timeout middleware is used to timeout at a long running operation within a predefined period. | |||
|
|
|||
| When timeout occurs, and the client receives timeout response the handler keeps running its code and keeps using resources until it finishes and returns! | |||
There was a problem hiding this comment.
Sorry for posting a comment too late. When I read this the first time I was not sure if it's true, so after a while playing around with the Timeout middleware a found a bug in it. Please see labstack/echo#1909
So, I think this should say that the handler must handle the Request Context properly in order to finish when the middleware timeouts.
There was a problem hiding this comment.
no, only "reasonable" usecase for timeout middleware is to let handler execute until it normally finishes/exists regardless what happens with coroutine serving request. If this usecase is not needed - then timeout middleware is not needed. you could just create new context with timeout and be fine.
There was a problem hiding this comment.
"reasonable" i.e. if client disconnects prematurely the db transaction would still get finished and commited.
if we start cancelling contexts it would mean that conn.ExecContext(ctx, query, args) would get cancelled and work would not get commited.
There was a problem hiding this comment.
I should probably indicate that also in example that doBusinessLogic takes in will get deadlined.
There was a problem hiding this comment.
I think timeout has 2 properties that user may seek/need:
- to get response before long-running handler actually finishes (for them even getting http.StatusServiceUnavailable better than nothing)
- to let handler finish work regardless what happens to the client (for them getting unfinished work commited is essential)
There was a problem hiding this comment.
Well, for me the timeout middleware was to ensure that your handler do the work you want in a particular time period, and if that isn't true you get a response that indicates that. And for that reason all the work should be stop. But your descriptions are more like a middleware to do async work, and of course you are right, if that is the case the work must continue until it is finished. Maybe we need to decide which of this two behaviors will be supported by this middleware an rename it to clarify our intention if it is needed.
| } | ||
| ``` | ||
|
|
||
| ## Alternatively handle timeouts in handlers |
There was a problem hiding this comment.
I'm sure I'm missing something, because for me this is what the Timeout middleware is actually doing. Could you please help me to understand why this is better vs using the Timeout middleware?
There was a problem hiding this comment.
In my opinion handling work in background is not webserver responsibility and should be implemented explicitly in business domain (i.e. by developer). This ensures that people will not set e.Use(middleware.Timeout()) mindlessly without thinking what and why they actually do it.
| // in different coroutine that should not access echo.Context and response writer | ||
|
|
||
| log.Printf("uid: %v\n", UID) | ||
| //res, err := slowDatabaseCon.ExecContext(ctx, query, args) |
There was a problem hiding this comment.
I'll add note here. Using methods with contexts has maybe sideeffects that are not wanted - i.e. when timeout we set to request is reached, ctx will be cancelled and therefore methods with contexts could prematurely return
| return err | ||
| } | ||
| } | ||
| return c.NoContent(http.StatusAccepted) |
There was a problem hiding this comment.
If I got it correctly, this is returning a 202 even when doBusinessLogic finished. At this point the code should return 200 instead.
|
note to self: probably should add a note that order of middlewares is important. ala if |
update timeout middleware with warnings and add example how to implement similar functionality in handler function