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

Deal with backends that don't close connections #3886

Closed
wants to merge 8 commits into from

Conversation

walid-git
Copy link
Member

Add "expect_close" parameter to probe to deal with backends that are considered sick because they don't close the TCP connection after sending the response.
The new parameter accepts the value true or false, and defaults to true for backward compatibility.

Copy link
Member

@Dridi Dridi left a comment

Choose a reason for hiding this comment

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

LGTM!

Please add more coverage in bin/varnishtest/tests/v00005.vtc.

doc/sphinx/reference/vcl-probe.rst Outdated Show resolved Hide resolved
@Dridi
Copy link
Member

Dridi commented Jan 16, 2023

bugwash: @bsdphk OK, and I would like to see some changes mentioned above. Maybe @nigoroll wants to have a look too.

@nigoroll
Copy link
Member

Do I read the patch correctly, that it still waits for the timeout to expire to accept the backend response?
If yes, that might not be what people expect? At least I would expect the probe to succeed as soon as sufficient data has been read to judge on it

@Dridi
Copy link
Member

Dridi commented Jan 23, 2023

This is why I requested a warning in the documentation. It's much simpler to implement "expect_close=false" as a last chance after timeout.

Copy link
Member

@Dridi Dridi left a comment

Choose a reason for hiding this comment

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

I caught a few things I didn't notice in my preview review.

lib/libvcc/vcc_utils.c Show resolved Hide resolved
bin/varnishtest/tests/v00005.vtc Outdated Show resolved Hide resolved
bin/varnishtest/tests/c00113.vtc Outdated Show resolved Hide resolved
doc/sphinx/reference/vcl-probe.rst Outdated Show resolved Hide resolved
@Dridi
Copy link
Member

Dridi commented Feb 8, 2023

Reviewed and merged as 908c9b8...6f50b7c with @walid-git.

@Dridi Dridi closed this Feb 8, 2023
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.

None yet

3 participants