-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
try to fix #1905 and add some notes #1947
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1947 +/- ##
=======================================
Coverage 91.03% 91.04%
=======================================
Files 32 32
Lines 2800 2802 +2
=======================================
+ Hits 2549 2551 +2
Misses 160 160
Partials 91 91
Continue to review full report at Codecov.
|
@pwli0755 could you rebase this pr? |
Done! But it seems codecov reports a warning, does that matter? |
Add warning about middleware
So it seems that order of middlewares with timeout middleware is important. If timeout is after logger then we have data race because we change that response Writer around and some middleware use it to get Headers for example. My personal opinion: |
I'll merge this for now. @pwli0755 thanks for your PR and if you have ideas for this middleware let us know. |
Yeah, I think the HTTP client is more responsible for the timeout things. |
I totally agree and when semi "background processing" is needed this should be explicitly coded in handlers. |
This tiny modification tries to avoid issue #1905 and add some notes about the global error handler.
If there are any better solutions, please ping me.