fix: proxy filter panics did not run applyFiltersOnError#3969
Conversation
Example: In the proxy a route with filter chain a() -> b() exists. b() panics and a() implements errorHandlerFilter interface and return true from HandleErrorResponse(). In this case we should run the filter a().Response(), because we need to cleanup things as expected. For example scheduler filters like fifo() or lifo() need to decrement the active concurrent requests counter or we will stop requests in the queue, wich would be miss leading. If we panic in b() and everyone will look at behavior of a(), because we are stuck there caused by not running the cleanup. Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
madumalt
left a comment
madumalt
left a comment
There was a problem hiding this comment.
some minor comments, suggestions to change some variable names. The logic seems sound to me!
|
|
||
| // applies filters to a request | ||
| func (p *Proxy) applyFiltersToRequest(f []*routing.RouteFilter, ctx *context) []*routing.RouteFilter { | ||
| func (p *Proxy) applyFiltersToRequest(f []*routing.RouteFilter, ctx *context, counter *int) []*routing.RouteFilter { |
There was a problem hiding this comment.
shouldn't the var name be index instead of counter ?
There was a problem hiding this comment.
No because we count number of filters executed in the request path
There was a problem hiding this comment.
I got this whole idea that is more like an index is with this,
There was a problem hiding this comment.
I approved the PR, if the suggestion doen't make sense, just ignore it :)
|
|
||
| func (p *Proxy) do(ctx *context, parentSpan ot.Span) (err error) { | ||
| var requestElapsed, responseElapsed time.Duration | ||
| processedFiltersCounter := 0 |
There was a problem hiding this comment.
similar to the above comment, shouldn't this be something like lastProcessedFilterIndex ?
There was a problem hiding this comment.
No, same here and "last" would be misleading
Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
|
👍 |
| } | ||
|
|
||
| if processedFiltersCounter != 0 { | ||
| p.applyFiltersOnError(ctx, ctx.route.Filters[:processedFiltersCounter]) |
There was a problem hiding this comment.
What if we have another panic in this call?
szuecs
deleted the
fix/filter-panic-did-not-run-response-filters-on-error
branch
3 participants
fix #3966
Example: In the proxy a route with filter chain a() -> b() exists.
b() panics and a() implements errorHandlerFilter interface and return true from HandleErrorResponse(). In this case we should run the filter a().Response(), because we need to cleanup things as expected.
For example scheduler filters like fifo() or lifo() need to decrement the active concurrent requests counter or we will stop requests in the queue, wich would be miss leading. If we panic in b() and everyone will look at behavior of a(), because we are stuck there caused by not running the cleanup.