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

#766 fix proxy stream #787

Merged
merged 14 commits into from
Dec 1, 2023
Merged

#766 fix proxy stream #787

merged 14 commits into from
Dec 1, 2023

Conversation

malud
Copy link
Collaborator

@malud malud commented Nov 22, 2023

closes #766

Reviewer checklist
  • Read PR description: a summary about the changes is required
  • Changelog updated
  • Documentation: docs/{Reference, Cli, ...}, Docker and cli help/usage
  • Pulled branch, manually tested
  • Verified requirements are met
  • Reviewed the code
  • Reviewed the related tests

@malud malud changed the base branch from master to release.1.12 November 22, 2023 17:04
@malud malud changed the title [WIP][edge] 766 fix proxy stream #766 fix proxy stream Nov 22, 2023
@malud malud linked an issue Nov 22, 2023 that may be closed by this pull request
@johakoch johakoch self-requested a review November 23, 2023 14:59
johakoch
johakoch previously approved these changes Nov 23, 2023
Copy link
Collaborator

@johakoch johakoch left a comment

Choose a reason for hiding this comment

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

This solves the problem described in #766.
One observation: I tried the config from the issue and got cancels sometimes, e.g.

ERRO[0333] internal server error: body copy failed: context canceled  build=dev handler=endpoint type=couper_daemon uid=clfkru9s6vh37hl8dhs0 version=0

due to the backend timeout,

or

ERRO[0118] internal server error: body copy failed: read error during body copy: unexpected EOF  build=dev handler=endpoint type=couper_daemon uid=clfl6u1s6vhc5ia5ji2g version=0

. In both cases, the access log had status=200, which is, at least, misleading.


TestBackend_MaxConnections_BodyClose fails quite regularly:

$ go test -timeout 300s -race github.com/avenga/couper/server -run TestBackend_MaxConnections_BodyClose -count=1
INTEGRATION: create test backend...
--- FAIL: TestBackend_MaxConnections_BodyClose (71.03s)
    --- FAIL: TestBackend_MaxConnections_BodyClose/parallel (0.00s)
        --- FAIL: TestBackend_MaxConnections_BodyClose/parallel/_/proxy-seq (70.00s)
            http_backend_test.go:130: Get "http://couper.dev:8080/proxy-seq": context deadline exceeded
        --- FAIL: TestBackend_MaxConnections_BodyClose/parallel/_/proxy-seq-ref (70.01s)
            http_backend_test.go:130: Get "http://couper.dev:8080/proxy-seq-ref": context deadline exceeded
        --- FAIL: TestBackend_MaxConnections_BodyClose/parallel/_/ws (70.01s)
            http_backend_test.go:130: Get "http://couper.dev:8080/ws": context deadline exceeded
        --- FAIL: TestBackend_MaxConnections_BodyClose/parallel/_/default2 (70.01s)
            http_backend_test.go:130: Get "http://couper.dev:8080/default2": context deadline exceeded
        --- FAIL: TestBackend_MaxConnections_BodyClose/parallel/_/ (70.01s)
            http_backend_test.go:130: Get "http://couper.dev:8080/": context deadline exceeded
        --- FAIL: TestBackend_MaxConnections_BodyClose/parallel/_/default (70.01s)
            http_backend_test.go:130: Get "http://couper.dev:8080/default": context deadline exceeded
        --- FAIL: TestBackend_MaxConnections_BodyClose/parallel/_/named (70.01s)
            http_backend_test.go:130: Get "http://couper.dev:8080/named": context deadline exceeded

@johakoch johakoch dismissed their stale review November 24, 2023 08:01

TestBackend_MaxConnections_BodyClose fails too often. This test (even with the same go code ported to release.1.12) passes 10/10.

@johakoch
Copy link
Collaborator

johakoch commented Nov 24, 2023

If I change the paths to

	paths := []string{
		"/named",
		"/named",
		"/named",
		"/named",
	}

(which doesn't have default request/proxy), TestBackend_MaxConnections_BodyClose passes 10/10.

If I change it to

	paths := []string{
		"/",
		"/",
		"/",
		"/",
	}

(which has default request), TestBackend_MaxConnections_BodyClose fails 8/10.

So I guess, this is related to the changes to the handling of default requests/proxies in this PR.

In Backend.innerRoundTrip(), all backend requests are passed to

	beresp, err := b.transport.RoundTrip(req)

but at least some get a "context canceled" error.

@johakoch
Copy link
Collaborator

The test started to fail with commit "Fix buffer condition and related body parse in combination with syncedVariables provided by each backend instance", then passed with commit "Fix random order related to the default block while preparing roundtrips" and failed again with commit "reduce produce wait-time; TODO".

Apparently, the

		time.Sleep(time.Nanosecond * 10) // TODO: refactor to ch pipe

is not stable enought. The previous

		time.Sleep(time.Millisecond * 5) // TODO: refactor to ch pipe

was more stable.

@malud malud marked this pull request as ready for review November 28, 2023 16:14
johakoch
johakoch previously approved these changes Dec 1, 2023
@malud malud merged commit 966f07d into release.1.12 Dec 1, 2023
1 check passed
@malud malud deleted the 766-fix-proxy-stream branch December 1, 2023 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streaming a backend response-body seems to be broken with proxy.
2 participants