Skip to content

Rely on request.Context() cancellation#38284

Merged
thaJeztah merged 2 commits intomoby:masterfrom
cpuguy83:context_in_api
Nov 28, 2018
Merged

Rely on request.Context() cancellation#38284
thaJeztah merged 2 commits intomoby:masterfrom
cpuguy83:context_in_api

Conversation

@cpuguy83
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 commented Nov 27, 2018 •
edited
Loading

The cancellable handler is no longer needed as the context that is
passed with the http request will be cancelled just like the close
notifier was doing.

Also updates gorilla/mux... This fixes an issue with mux usage of context for storing vars... took me a bit to figure out that calling mux.Vars(r) after I added my own context to r was messing everything up.
Also the old version is 2 years old.

@GordonTheTurtle GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature status/0-triage labels Nov 27, 2018
@cpuguy83 cpuguy83 changed the title Rely on reqest.Context cancellation Rely on request.Context() cancellation Nov 27, 2018
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Nov 27, 2018
thaJeztah
thaJeztah approved these changes Nov 28, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 28, 2018

Hm failure; could this be due to a change in behavior in gorilla/mux? I recall we ran into this at some point, perhaps another attempt to update it

22:48:15 FAIL: docker_api_test.go:91: DockerSuite.TestAPIErrorNotFoundJSON
22:48:15 
22:48:15 docker_api_test.go:95:
22:48:15     c.Assert(httpResp.StatusCode, checker.Equals, http.StatusNotFound)
22:48:15 ... obtained int = 405
22:48:15 ... expected int = 404
22:48:15 
22:48:15

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 28, 2018

could've been this one #30933 🤔 but think I had another one in mind

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 28, 2018

Ah! This was the PR #37522 (comment)

Same failure

@cpuguy83
Copy link
Copy Markdown
Member Author

cpuguy83 commented Nov 28, 2018

Damnit.

@cpuguy83
Update gorilla/mux
d5916f6 
This fixes an issue with mux usage of context for storing vars.
Also the old version is 2 years old.

Signed-off-by: Brian Goff <[email protected]>
@cpuguy83
Rely on request.Context() cancellation
05390c4 
The cancellable handler is no longer needed as the context that is
passed with the http request will be cancelled just like the close
notifier was doing.

Signed-off-by: Brian Goff <[email protected]>
@cpuguy83
Copy link
Copy Markdown
Member Author

cpuguy83 commented Nov 28, 2018

Fixed 🤞

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 28, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@d3e75e4). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #38284   +/-   ##
=========================================
  Coverage          ?   36.14%           
=========================================
  Files             ?      610           
  Lines             ?    45285           
  Branches          ?        0           
=========================================
  Hits              ?    16367           
  Misses            ?    26681           
  Partials          ?     2237

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 28, 2018

I think I looked into that at the time with @vdemeester and it was because non-existing endpoints were still handled somewhere in our code, but it's been a while 🤔

cpuguy83
cpuguy83 commented Nov 28, 2018
View reviewed changes
Comment thread api/server/server.go
notFoundHandler := httputils.MakeErrorHandler(pageNotFoundError{})
m.HandleFunc(versionMatcher+"/{path:.*}", notFoundHandler)
m.NotFoundHandler = notFoundHandler
m.MethodNotAllowedHandler = notFoundHandler
Copy link
Copy Markdown
Member Author

@cpuguy83 cpuguy83 Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gorilla/mux now uses a separate handler for non-existent routes. This changes it back to the old behavior.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! nice find; perhaps worth adding a comment, explaining why we do this?

Copy link
Copy Markdown
Member Author

@cpuguy83 cpuguy83 Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test enforcing it is enough, also git blame.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okz

thaJeztah
thaJeztah approved these changes Nov 28, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still LGTM (if green)

@moby moby deleted a comment from GordonTheTurtle Nov 28, 2018
vdemeester
vdemeester approved these changes Nov 28, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if green 🍏

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 28, 2018

It's green (Ignoring codecov/patch) 🎉

@thaJeztah thaJeztah merged commit 852542b into moby:master Nov 28, 2018
@cpuguy83 cpuguy83 deleted the context_in_api branch November 28, 2018 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

+1 more reviewer

@vdemeester vdemeester vdemeester approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cpuguy83 @thaJeztah @vdemeester @GordonTheTurtle