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

waiter: report WAITER_REMCLOSE correctly #4138

Merged
merged 4 commits into from
Aug 21, 2024

Conversation

asadsa92
Copy link
Contributor

There seems to be a discrepancy between the epoll and kqueue
implementation. One implementation is not reporting WAITER_REMCLOSE,
while the other implementation is reporting WAITER_REMCLOSE too soon.

epoll:
We sometimes incorrectly report WAITER_ACTION when it is supposed to be
WAITER_REMCLOSE. We can check EPOLLRDHUP before reading 1 byte with MSG_PEEK to
detect WAITER_REMCLOSE. This saves us from a pontential session detour from the
Waiter to a worker thread.

kqueue:
We incorrectly report WAITER_REMCLOSE when there could be more data to be
read from the socket. Change this so that we perform read of 1 byte with
MSG_PEEK to detect closure when EV_EOF is set.

DESCRIPTION:EVFILT_READ:Sockets:

It is possible for EOF to be returned (indicating the connection is gone)
while there is still data pending in the socket buffer.

nigoroll added a commit that referenced this pull request Jul 29, 2024
"999 pools ought to be enough for everyone" yet the actual maximum is UINT_MAX.

Noticed while staring at #4138
@asadsa92
Copy link
Contributor Author

asadsa92 commented Aug 12, 2024

bugwash: Ok to merge. But, we should keep an eye on it w.r.t regression.

Introduce counters that can be used by the Waiter.

Signed-off-by: Asad Sajjad Ahmed <asadsa@varnish-software.com>
Make the Waiter more transparent from the outside.

Signed-off-by: Asad Sajjad Ahmed <asadsa@varnish-software.com>
Let a client or server perform a graceful shutdown.
By default, we close both directions.
Options can be given to only close read or write direction.

Signed-off-by: Asad Sajjad Ahmed <asadsa@varnish-software.com>
There seems to be a discrepancy between the epoll and kqueue
implementation. One implementation is not reporting WAITER_REMCLOSE,
while the other implementation is reporting WAITER_REMCLOSE too soon.

epoll:
We sometimes incorrectly report WAITER_ACTION when it is supposed to be
WAITER_REMCLOSE. We can check EPOLLRDHUP before reading 1 byte with MSG_PEEK to
detect WAITER_REMCLOSE. This saves us from a pontential session detour from the
Waiter to a worker thread.

kqueue:
We incorrectly report WAITER_REMCLOSE when there could be more data to be
read from the socket. Change this so that we perform read of 1 byte with
MSG_PEEK to detect closure when EV_EOF is set.

DESCRIPTION:EVFILT_READ:Sockets:
"It is possible for EOF to be  returned (indicating the connection is gone)
while there is still data pending in the socket buffer."
- kqueue (2)

Signed-off-by: Asad Sajjad Ahmed <asadsa@varnish-software.com>
@asadsa92
Copy link
Contributor Author

Rebased to solve a filename conflict for one of the test case.
Should be ready to merge after the CCI passes.

@asadsa92 asadsa92 merged commit 2bb425f into varnishcache:master Aug 21, 2024
10 of 11 checks passed
@asadsa92 asadsa92 deleted the epollrdhup branch August 21, 2024 09:52
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.

2 participants