Move timeout handling into handlers.

[winder]

Summary

Replace echo TimeoutMiddleware with timeout handling in handlers. The behavior is functionally identical to the previous implementation, but no longer contains concurrency primitives spanning multiple handler functions.

Test Plan

• New unit tests for each handler timing out.
• Unit test for CallWithTimeout utility.
• make integration, make e2e, and integration go tests all use this code path.

[tolikzinovyev]

What does this fix?

[winder]

What does this fix?

There are strange race conditions that cause response writers to get mixed up. It seems to be a result of the TimeoutMiddleware, I was able to reproduce some strange behaviors and panics in a unit test.

There are many discussions on github about people finding different data race conditions, and pretty much giving up on the middleware. Here is one: labstack/echo#1947 (comment)

[tolikzinovyev]

Thanks for the link. Why not do this? https://github.com/labstack/echo/pull/1947/files#diff-d2f7e13f33e9dc860e88548eb3886115c7876523f7b030da18a6f157663e2487R20

[winder]

I updated to the latest version of echo and was still able to reproduce some problems with this middleware. After disabling the middleware all issues went away.

The way echo passes around the shared context is very convenient for most normal HTTP server things, but seems to be problematic if the middleware needs to do things later on. In this case, after a timeout.

[tolikzinovyev]

But the example I pointed out is a custom middleware. Does it also not work?

[winder]

That custom middleware is what we were using. There are some rare race conditions in it.

[tolikzinovyev]

No, we were using a middleware from the github.com/labstack/echo/v4/middleware package. I'm talking about a custom middleware from the example.

[winder]

@tolikzinovyev I see. Yes, I'm using context.WithTimeout like this example. I don't think there's a readability benefit to splitting up the handling with part of it in the middleware and most of it still in the handler, so I put the whole thing in the handler.

I'm having some trouble getting context.WithTimoeut to work.

[codecov-commenter]

Codecov Report

Merging #786 (cd7c063) into develop (e45af39) will increase coverage by 4.15%.
The diff coverage is 57.02%.

@@             Coverage Diff             @@
##           develop     #786      +/-   ##
===========================================
+ Coverage    54.37%   58.53%   +4.15%     
===========================================
  Files           28       29       +1     
  Lines         3908     4001      +93     
===========================================
+ Hits          2125     2342     +217     
+ Misses        1499     1364     -135     
- Partials       284      295      +11     

Impacted Files	Coverage Δ
api/error_messages.go	100.00% <ø> (ø)
api/server.go	0.00% <0.00%> (ø)
api/handlers.go	58.84% <56.33%> (+34.88%)	⬆️
util/util.go	51.16% <70.00%> (ø)
api/pointer_utils.go	86.36% <0.00%> (+2.27%)	⬆️
api/converter_utils.go	90.42% <0.00%> (+3.38%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e45af39...cd7c063. Read the comment docs.

[gmalouf]

@winder can you add in PR description how you validated/tested the change / what the behavior is intended to do?

[tolikzinovyev]

this shouldn't be necessary also

[winder]

I'm OK with this one, it ensures that there's no panic if a subsequent check assumes the error is not nil. Maybe ErrorIs handles the nil case, or maybe not.