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

Receiving response even after the timeoutSeconds is reached #15486

Open
karanibm6 opened this issue Aug 28, 2024 · 5 comments · May be fixed by #15630
Open

Receiving response even after the timeoutSeconds is reached #15486

karanibm6 opened this issue Aug 28, 2024 · 5 comments · May be fixed by #15630
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@karanibm6
Copy link

karanibm6 commented Aug 28, 2024

For a knative revision, one can send the response headers and some data before the timeout. It is then seemingly possible to write data longer than the timeout, possibly forever. This is imo not how Knative defines the timeout seconds:

$ kubectl explain ksvc.spec.template.spec.timeoutSeconds
GROUP:      serving.knative.dev
KIND:       Service
VERSION:    v1

FIELD: timeoutSeconds <integer>

DESCRIPTION:
    TimeoutSeconds is the maximum duration in seconds that the request instance
    is allowed to respond to a request. If unspecified, a system default will be
    provided.

What version of Knative?

v1.15.2

Expected Behavior

The request should timeout when the timeoutSeconds is reached.

Actual Behavior

The request doesn't timeout within timeoutSeconds

Steps to Reproduce the Problem

  1. Create a KSvc using the image ghcr.io/src2img/http-synthetics:latest (code is from https://github.com/src2img/http-synthetics), Use minScale and maxScale 1. and set timeoutSeconds to 30s
  2. Wait for the Ksvc to become ready.
  3. Call its endpoint with curl -X PUT http://endpoint/write-regularly?interval=10&count=5

You will observe that you will get the response after 50s. and the request doesn't times out in 30s.

@karanibm6 karanibm6 added the kind/bug Categorizes issue or PR as related to a bug. label Aug 28, 2024
@skonto skonto changed the title Recieving response even after the timeoutSeconds is reached Receiving response even after the timeoutSeconds is reached Oct 7, 2024
@skonto
Copy link
Contributor

skonto commented Oct 8, 2024

Hi @karanibm6 the timeout is based on whether a write happened or not.
For example if a write does not happen within the timeout period the request expires:

curl -H "Host: http-synthetics.default.example.com" "http://192.168.39.242:31935/write-regularly?interval=40&count=1"
activator request timeout

That is the meaning of "... that the request instance is allowed to respond to a request."

@SaschaSchwarze0
Copy link
Contributor

@skonto thanks for looking into this. Though, what you describe sounds rather like the responseStartTimeoutSeconds timeout:

$ kubectl explain ksvc.spec.template.spec.responseStartTimeoutSeconds
GROUP:      serving.knative.dev
KIND:       Service
VERSION:    v1

FIELD: responseStartTimeoutSeconds <integer>

DESCRIPTION:
    ResponseStartTimeoutSeconds is the maximum duration in seconds that the
    request
    routing layer will wait for a request delivered to a container to begin
    sending any network traffic.

What would be the purpose of timeout if it behaves like responseStartTimeoutSeconds ?

@skonto skonto linked a pull request Nov 26, 2024 that will close this issue
@skonto
Copy link
Contributor

skonto commented Nov 26, 2024

@SaschaSchwarze0 hi,

This is indeed confusing. I see that we have a conformance test:

{
		name:           "writes first byte before timeout",
		timeoutSeconds: 10,
		expectedStatus: http.StatusOK,
		sleep:          15 * time.Second,
		initialSleep:   0,
	}

The logic in the test image does the following:

	// Explicitly flush the already written data to trigger (or not)
	// the time-to-first-byte timeout.
	if f, ok := w.(http.Flusher); ok {
		f.Flush()
	}

	// Sleep for a set amount of time before sending response.
	timeout, _ := strconv.Atoi(r.URL.Query().Get("timeout"))
	time.Sleep(time.Duration(timeout) * time.Millisecond)

	fmt.Fprintf(w, "Slept for %d milliseconds", timeout)

This by design, allows to bypass the timeoutSeconds value as reported in the issue here.

However, in the past we changed the spec back to its original idea of using timeoutSeconds as maxDuration since it had deviated: see #12970 and knative/specs#100. So it seems we should just timeout when the response is not within the timeoutSeconds defined but we don't comply with that? 🤔 cc @dprotaso what is the status here?

@skonto
Copy link
Contributor

skonto commented Nov 28, 2024

I have a PR that tests enforcing the timeout. Note that once we write the header we can't re-write it, but behavior is enforced to kill the connection.

@dprotaso
Copy link
Member

So it seems we should just timeout when the response is not within the timeoutSeconds defined but we don't comply with that

The request should be killed - I'm guessing we should track if we've written a header and not do it again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@dprotaso @skonto @SaschaSchwarze0 @karanibm6 and others