handler: fix panic in hook emitting logic #1337
Conversation
Acconut
left a comment
Acconut
left a comment
There was a problem hiding this comment.
Thank you very for digging into this and finding the culprit! I just have two remarks and then we can merge this.
pkg/handler/hooks.go
Outdated
| c.req.Header.Set("Host", c.req.Host) | ||
|
|
||
| return HookEvent{ | ||
| Context: c, | ||
| Upload: info, | ||
| HTTPRequest: HTTPRequest{ | ||
| Method: c.req.Method, | ||
| URI: c.req.RequestURI, | ||
| RemoteAddr: c.req.RemoteAddr, | ||
| Header: c.req.Header, | ||
| Header: c.req.Header.Clone(), |
There was a problem hiding this comment.
It would be better if we don't modify the request header at all, but just add the Host entry to a copy for the hooks:
| Header: c.req.Header.Clone(), | |
| reqHeader := c.req.Header.Clone() | |
| reqHeader.Set("Host", c.req.Host) | |
| return HookEvent{ | |
| Context: c, | |
| Upload: info, | |
| HTTPRequest: HTTPRequest{ | |
| Method: c.req.Method, | |
| URI: c.req.RequestURI, | |
| RemoteAddr: c.req.RemoteAddr, | |
| Header: reqHeader, |
There was a problem hiding this comment.
I agree with that. I would even put it into a separate field so that we don't need to copy but it would be breaking change in serialization so for the fix, updated not to modify original headers.
| // This test will fail with `go test -race` if Header is not cloned. | ||
| // | ||
| // See: https://github.com/tus/tusd/issues/1320 | ||
| func TestHookEventHeaderRace(t *testing.T) { |
There was a problem hiding this comment.
Thank you for adding these tests as a demonstration. While they are useful now, I'm not sure if we should keep them in the repo after the fix is merged.
There was a problem hiding this comment.
IMO, they are useful to catch if there might be a regression. It doesn't complicate or slow down, better to keep but if you insist, I can drop so that we can merge.
fixes tus#1320 Signed-off-by: ferhat elmas <[email protected]>
ferhatelmas
force-pushed
the
hook-race
branch
from
82a8808 to
f73c97e
Compare
Acconut
changed the title
2 participants
fixes #1320