forked from postgres/postgres
-
Notifications
You must be signed in to change notification settings - Fork 0
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
jchampio
wants to merge
11
commits into
master
Choose a base branch
from
dev/oauth-kqueue
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Dev/oauth kqueue #20
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bb5a8ed
to
347cdae
Compare
8a1013f
to
c76edfa
Compare
2e16a38
to
de7b1ab
Compare
e9f2357
to
f8dde8b
Compare
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.
f79ec1f
to
e3756e1
Compare
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.
e3756e1
to
3856f5d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.