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

Revision timeoutSecond should only apply until the first byte of the response #2582

Closed
tanzeeb opened this issue Nov 29, 2018 · 3 comments
Closed
Assignees
Labels
area/API API objects and controllers area/networking kind/bug Categorizes issue or PR as related to a bug. kind/spec Discussion of how a feature should be exposed to customers.
Milestone

Comments

@tanzeeb
Copy link
Contributor

tanzeeb commented Nov 29, 2018

Expected Behavior

When I deploy an application with the timeoutSeconds, I expect the timeout to only apply from when the request is sent until the response response begins.

Actual Behavior

When I deploy an application with the timeoutSeconds set, eg. "45s", I the timeout applies from when the request is sent until the response response ends.

Steps to Reproduce the Problem

  1. Modify the knative/serving/test/test_images/timeout application as follows:
func handler(w http.ResponseWriter, r *http.Request) {
	before, _ := strconv.Atoi(r.URL.Query().Get("before"))
	after, _ := strconv.Atoi(r.URL.Query().Get("after"))
	time.Sleep(time.Duration(before) * time.Second)
        fmt.Fprintf(w, "Begin after %d seconds", before)
        time.Sleep(time.Duration(after) * time.Second)
	fmt.Fprintf(w, "End after %d seconds", after)
}
  1. Deploy with the following service.yaml:
apiVersion: serving.knative.dev/v1alpha1
kind: Service
metadata:
  name: timeout
  namespace: default
spec:
  runLatest:
    configuration:
      revisionTemplate:
        spec:
          timeoutSeconds: "30s"
          container:
            image: github.com/knative/serving/test/test_images/timeout
  1. Curl the service a query parameter less than the timeout ?before=15&after=45. This will fail, but should not fail.
  2. Curl the service a query parameter less than the timeout ?before=45&after=15. This will fail, and should fail.

Additional Info

The current implementation uses http.TimeoutHandler, which will buffer the entire request. This poses problems for streaming requests, eg. gRPC in #2539.

cc @tcnghia @mattmoor @evankanderson

@knative-prow-robot knative-prow-robot added area/API API objects and controllers area/networking kind/bug Categorizes issue or PR as related to a bug. kind/spec Discussion of how a feature should be exposed to customers. labels Nov 29, 2018
@mattmoor
Copy link
Member

mattmoor commented Dec 5, 2018

@tanzeeb Is this something you think you can tackle for 0.3?

@mattmoor mattmoor added this to the Serving 0.3 milestone Dec 5, 2018
@mattmoor
Copy link
Member

mattmoor commented Dec 5, 2018

cc @tcnghia too FYI

@mattmoor
Copy link
Member

mattmoor commented Dec 6, 2018

/assign @markusthoemmes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API API objects and controllers area/networking kind/bug Categorizes issue or PR as related to a bug. kind/spec Discussion of how a feature should be exposed to customers.
Projects
None yet
Development

No branches or pull requests

4 participants