Apply timeout in the queue proxy on 'time to first byte'. #2696
Conversation
knative-prow-robot
added
the
size/L
There was a problem hiding this comment.
@markusthoemmes: 0 warnings.
In response to this:
Fixes #2582
Proposed Changes
- Implemented a custom timeout handler (based on http.TimeoutHandler), that applies a timeout, if no bytes have been written to the connection for
timeouttime.@tanzeeb this includes setting the StatusCode at the moment, but we can strip that out to only include
Writecalls, if that makes more sense?Release Note
The revision's timeout is now interpreted as 'time to first byte', to support streaming cases.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
knative-prow-robot
added
the
approved
|
/test pull-knative-serving-go-coverage |
|
/test pull-knative-serving-go-coverage |
|
/test pull-knative-serving-integration-tests |
| for { | ||
| select { | ||
| case p := <-panicChan: | ||
| panic(p) |
There was a problem hiding this comment.
I believe this leaks done on this and the timeout path.
There was a problem hiding this comment.
I think it only leaks here. The timeout path cancels the context. That doesn't make it panic so the handler's close(done) call is properly reached. To be sure, I tried adding a close call to the timeout path and that errored due to multiple close calls.
| done := make(chan struct{}) | ||
| // The recovery value of a panic is written to this channel to be | ||
| // propagated (panicked with) again. | ||
| panicChan := make(chan interface{}, 1) |
There was a problem hiding this comment.
https://stackoverflow.com/a/8593986/3269863 suggests that channels don't need to be closed (that is: if you don't need to use the close as a signal as well).
I still pushed a commit to close the channels appropriately.
knative-prow-robot
removed
the
approved
|
The following is the coverage report on pkg/.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markusthoemmes, mattmoor The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
Fixes #2582
Proposed Changes
timeouttime.@tanzeeb this includes setting the StatusCode at the moment, but we can strip that out to only include
Writecalls, if that makes more sense?Release Note