Skip to content

Dev/oauth kqueue #20

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Dev/oauth kqueue #20

wants to merge 11 commits into from

Conversation

jchampio
Copy link
Owner

No description provided.

@jchampio jchampio force-pushed the dev/oauth-kqueue branch 2 times, most recently from bb5a8ed to 347cdae Compare May 22, 2025 23:43
@jchampio jchampio force-pushed the dev/oauth-kqueue branch 5 times, most recently from 8a1013f to c76edfa Compare June 6, 2025 22:48
jchampio added 2 commits June 24, 2025 12:00
In b0635bf, I added an early header check to the Meson OAuth support,
which was intended to duplicate the later checks for
HAVE_SYS_[EVENT|EPOLL]_H. However, I implemented the new test via
check_header() -- which tries to compile -- rather than has_header(),
which just looks for the file's existence.

The distinction matters on OpenBSD, where <sys/event.h> can't be
compiled without including prerequisite headers, so -Dlibcurl=enabled
failed on that platform. Switch to has_header() to fix this.
@jchampio jchampio force-pushed the dev/oauth-kqueue branch 7 times, most recently from f79ec1f to e3756e1 Compare June 26, 2025 18:34
jchampio and others added 9 commits June 26, 2025 15:25
If a socket is added to the kqueue, becomes readable/writable, and
subsequently becomes non-readable/writable again, the kqueue itself will
remain readable until either the socket registration is removed, or the
stale event is cleared via a call to kevent().

In many simple cases, Curl itself will remove the socket registration
quickly, but in real-world usage, this is not guaranteed to happen. The
kqueue can then remain stuck in a permanently readable state until the
request ends, which results in pointless wakeups for the client and
wasted CPU time.

Implement drain_socket_events() to call kevent() and unstick any stale
events. This is called right after drive_request(), before we return
control to the client to wait. To make sure we've taken a look at the
entire queue, register_socket() now tracks the number of outstanding
registrations.

Suggested-by: Thomas Munro <thomas.munro@gmail.com>
In a case similar to the previous commit, an expired timer can remain
permanently readable if Curl does not remove the timeout itself. Since
that removal isn't guaranteed to happen in real-world situations,
implement drain_timer_events() to reset the timer before calling into
drive_request().

Moving to drain_timer_events() happens to fix a logic bug in the
previous caller of timer_expired(), which treated an error condition as
if the timer were expired instead of bailing out.

The previous implementation of timer_expired() gave differing results
for epoll and kqueue if the timer was reset. (For epoll, a reset timer
was considered to be expired, and for kqueue it was not.) This didn't
previously cause problems, since timer_expired() was only called while
the timer was known to be set, but both implementations now use the
kqueue logic.
Tracking down the bugs that led to the addition of drain_socket_events()
and drain_timer_events() was difficult, because an inefficient flow is
not visibly different from one that is working properly. To help
maintainers notice when something has gone wrong, track the number of
calls into the flow as part of debug mode, and print the total when the
flow finishes.

A new test makes sure the total count is less than 100. (We expect
something on the order of 10.) This isn't foolproof, but it is able to
catch several regressions in the logic of the prior two commits, and
future work to add TLS support to the oauth_validator test server should
strengthen it as well.
To better record the internal behaviors of oauth-curl.c, add a unit test
suite for the socket and timer handling code. This is all based on TAP
and driven by our existing Test::More infrastructure.
Requires Python 3. On the first run of `make installcheck` or `meson
test` the dependencies will be installed into a local virtualenv for
you. See the README for more details.

Cirrus has been updated to build OAuth support on Debian and FreeBSD.

The suite contains a --temp-instance option, analogous to pg_regress's
option of the same name, which allows an ephemeral server to be spun up
during a test run.

TODOs:
- The --tap-stream option to pytest-tap is slightly broken during test
  failures (it suppresses error information), which impedes debugging.
- The with_oauth test skip logic should probably be integrated into the
  Makefile side as well...
- See if 32-bit tests can be enabled with a 32-bit Python.
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.

1 participant