Dev/oauth kqueue #20
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
jchampio
force-pushed
the
dev/oauth-kqueue
branch
2 times, most recently
from
bb5a8ed to
347cdae
Compare
jchampio
force-pushed
the
dev/oauth-kqueue
branch
5 times, most recently
from
8a1013f to
c76edfa
Compare
jchampio
force-pushed
the
dev/oauth-kqueue
branch
from
2e16a38 to
de7b1ab
Compare
jchampio
force-pushed
the
dev/oauth-kqueue
branch
9 times, most recently
from
e3756e1 to
3856f5d
Compare
jchampio
force-pushed
the
dev/oauth-kqueue
branch
4 times, most recently
from
7af4094 to
2afee4f
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 comb_multiplexer() to call kevent() and unstick any stale events that would cause unnecessary callbacks. This is called right after drive_request(), before we return control to the client to wait. Suggested-by: Thomas Munro <thomas.munro@gmail.com> Co-authored-by: Thomas Munro <thomas.munro@gmail.com> Reviewed-by: Thomas Munro <thomas.munro@gmail.com> Backpatch-through: 18 Discussion: https://postgr.es/m/CAOYmi+nDZxJHaWj9_jRSyf8uMToCADAmOfJEggsKW-kY7aUwHA@mail.gmail.com
If Curl needs to switch the direction of a socket's registration (e.g. from CURL_POLL_IN to CURL_POLL_OUT), it expects the old registration to be discarded. For epoll, this happened via EPOLL_CTL_MOD, but for kqueue, the old registration would remain if it was not explicitly removed by Curl. Explicitly remove the opposite-direction event during registrations. (If that event doesn't exist, we'll just get an ENOENT, which will be ignored by the same code that handles CURL_POLL_REMOVE.) A few assertions are also added to strengthen the relationship between the number of events added, the number of events pulled off the queue, and the lengths of the kevent arrays. Reviewed-by: Thomas Munro <thomas.munro@gmail.com> Backpatch-through: 18 Discussion: https://postgr.es/m/CAOYmi+nDZxJHaWj9_jRSyf8uMToCADAmOfJEggsKW-kY7aUwHA@mail.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. Reviewed-by: Thomas Munro <thomas.munro@gmail.com> Backpatch-through: 18 Discussion: https://postgr.es/m/CAOYmi+nDZxJHaWj9_jRSyf8uMToCADAmOfJEggsKW-kY7aUwHA@mail.gmail.com
Tracking down the bugs that led to the addition of comb_multiplexer() 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. Backpatch-through: 18 Discussion: https://postgr.es/m/CAOYmi+nDZxJHaWj9_jRSyf8uMToCADAmOfJEggsKW-kY7aUwHA@mail.gmail.com
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. Reviewed-by: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> Discussion: https://postgr.es/m/CAOYmi+nDZxJHaWj9_jRSyf8uMToCADAmOfJEggsKW-kY7aUwHA@mail.gmail.com
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.
jchampio
force-pushed
the
dev/oauth-kqueue
branch
from
2afee4f to
b0306b2
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.