Skip to content
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

Merged
merged 4 commits into from Aug 22, 2021
Merged

try to fix #1905 and add some notes #1947

merged 4 commits into from Aug 22, 2021

Conversation

pwli0755
Copy link
Contributor

@pwli0755 pwli0755 commented Aug 2, 2021

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.

@codecov
Copy link

codecov bot commented Aug 2, 2021

Codecov Report

Merging #1947 (c302290) into master (7d41537) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
middleware/timeout.go 100.00% <100.00%> (ø)

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 7d41537...c302290. Read the comment docs.

@aldas
Copy link
Contributor

aldas commented Aug 12, 2021

@pwli0755 could you rebase this pr?

lipengwei added 2 commits August 13, 2021 13:06
(cherry picked from commit 9d96199)
(cherry picked from commit e8ea1bc)
@pwli0755
Copy link
Contributor Author

pwli0755 commented Aug 13, 2021

@pwli0755 could you rebase this pr?

Done! But it seems codecov reports a warning, does that matter?

@aldas
Copy link
Contributor

aldas commented Aug 22, 2021

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:
sigh I really dislike this middleware because it so complex and has so many hidden "features" and I'm too dumb to fix all those problems in a reasonable way.

@aldas aldas merged commit 7f502b1 into labstack:master Aug 22, 2021
@aldas
Copy link
Contributor

aldas commented Aug 22, 2021

I'll merge this for now. @pwli0755 thanks for your PR and if you have ideas for this middleware let us know.

@pwli0755
Copy link
Contributor Author

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:
sigh I really dislike this middleware because it so complex and has so many hidden "features" and I'm too dumb to fix all those problems in a reasonable way.

Yeah, I think the HTTP client is more responsible for the timeout things.

@aldas
Copy link
Contributor

aldas commented Aug 23, 2021

I totally agree and when semi "background processing" is needed this should be explicitly coded in handlers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants