On Thu, Aug 7, 2025 at 11:11 AM Jacob Champion <[email protected]> wrote: > Thank you so much for the reviews!
Here is v4, with the feedback from both of you. 0001-0004 are planned for backport; 0005 is slated for master only. Thanks again for the reviews! --Jacob
1: c5cdccfe374 ! 1: a515435d3b4 oauth: Remove stale events from the kqueue
multiplexer
@@ Commit message
after drive_request(), before we return control to the client to wait.
Suggested-by: Thomas Munro <[email protected]>
+ Co-authored-by: Thomas Munro <[email protected]>
+ Reviewed-by: Thomas Munro <[email protected]>
+ Backpatch-through: 18
+ Discussion:
https://postgr.es/m/caoymi+ndzxjhawj9_jrsyf8umtocadamofjeggskw-ky7au...@mail.gmail.com
## src/interfaces/libpq-oauth/oauth-curl.c ##
@@ src/interfaces/libpq-oauth/oauth-curl.c: register_socket(CURL *curl,
curl_socket_t socket, int what, void *ctx,
#endif
}
-+/*-------
++/*
+ * If there is no work to do on any of the descriptors in the
multiplexer, then
+ * this function must ensure that the multiplexer is not readable.
+ *
-+ * As a motivating example, consider the following sequence of events:
-+ * 1. libcurl tries to write data to the send buffer, but it fills up.
-+ * 2. libcurl registers CURL_POLL_OUT on the socket and returns control
to the
-+ * client to wait.
-+ * 3. The kernel partially drains the send buffer. The socket becomes
writable,
-+ * and the client wakes up and calls back into the flow.
-+ * 4. libcurl continues writing data to the send buffer, but it fills up
again.
-+ * The socket is no longer writable.
-+ *
-+ * At this point, an epoll-based mux no longer signals readiness, so
nothing
-+ * further needs to be done. But a kqueue-based mux will continue to
signal
-+ * "ready" until either the EVFILT_WRITE registration is dropped for the
socket,
-+ * or the old socket-writable event is read from the queue. Since Curl
isn't
-+ * guaranteed to do the former, we must do the latter here.
++ * Unlike epoll descriptors, kqueue descriptors only transition from
readable to
++ * unreadable when kevent() is called and finds nothing, after removing
++ * level-triggered conditions that have gone away. We therefore need a
dummy
++ * kevent() call after operations might have been performed on the
monitored
++ * sockets or timer_fd. Any event returned is ignored here, but it also
remains
++ * queued (being level-triggered) and leaves the descriptor readable.
This is a
++ * no-op for epoll descriptors.
+ */
+static bool
+comb_multiplexer(struct async_ctx *actx)
@@ src/interfaces/libpq-oauth/oauth-curl.c:
pg_fe_run_oauth_flow_impl(PGconn *conn)
+ * Make sure that stale events don't
cause us to come back
+ * early. (Currently, this can occur
only with kqueue.) If
+ * this is forgotten, the multiplexer
can get stuck in a
-+ * signalled state and we'll burn CPU
cycles pointlessly.
++ * signaled state and we'll burn CPU
cycles pointlessly.
+ */
+ if (!comb_multiplexer(actx))
+ goto error_return;
2: 7725e0c173b ! 2: a34be19f17f oauth: Ensure unused socket registrations are
removed
@@ Commit message
number of events added, the number of events pulled off the queue, and
the lengths of the kevent arrays.
+ Reviewed-by: Thomas Munro <[email protected]>
+ Backpatch-through: 18
+ Discussion:
https://postgr.es/m/caoymi+ndzxjhawj9_jrsyf8umtocadamofjeggskw-ky7au...@mail.gmail.com
+
## src/interfaces/libpq-oauth/oauth-curl.c ##
@@ src/interfaces/libpq-oauth/oauth-curl.c: register_socket(CURL *curl,
curl_socket_t socket, int what, void *ctx,
3: 6ccf7a5d156 ! 3: 7408778d579 oauth: Remove expired timers from the
multiplexer
@@ Commit message
the timer was known to be set, but both implementations now use the
kqueue logic.
+ Reviewed-by: Thomas Munro <[email protected]>
+ Backpatch-through: 18
+ Discussion:
https://postgr.es/m/caoymi+ndzxjhawj9_jrsyf8umtocadamofjeggskw-ky7au...@mail.gmail.com
+
## src/interfaces/libpq-oauth/oauth-curl.c ##
@@ src/interfaces/libpq-oauth/oauth-curl.c: set_timer(struct async_ctx
*actx, long timeout)
4: 2be993b8f07 ! 4: 8241255e84c oauth: Track total call count during a client
flow
@@ Commit message
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-ky7au...@mail.gmail.com
+
## src/interfaces/libpq-oauth/oauth-curl.c ##
@@ src/interfaces/libpq-oauth/oauth-curl.c: struct async_ctx
bool user_prompted; /* have we already sent the authz
prompt? */
5: 50257bf32eb ! 5: 337124064f3 oauth: Add unit tests for multiplexer handling
@@ Commit message
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 <[email protected]>
+ Discussion:
https://postgr.es/m/caoymi+ndzxjhawj9_jrsyf8umtocadamofjeggskw-ky7au...@mail.gmail.com
+
## src/interfaces/libpq-oauth/Makefile ##
@@ src/interfaces/libpq-oauth/Makefile: uninstall:
rm -f '$(DESTDIR)$(libdir)/$(stlib)'
@@ src/interfaces/libpq-oauth/t/001_oauth.pl (new)
+my $err = $builder->failure_output;
+
+IPC::Run::run ['oauth_tests'],
-+ '>', IPC::Run::new_chunker, sub { print {$out} $_[0] },
-+ '2>', IPC::Run::new_chunker, sub { print {$err} $_[0] }
++ '>', IPC::Run::new_chunker, sub { $out->print($_[0]) },
++ '2>', IPC::Run::new_chunker, sub { $err->print($_[0]) }
+ or die "oauth_tests returned $?";
## src/interfaces/libpq-oauth/test-oauth-curl.c (new) ##
v4-0001-oauth-Remove-stale-events-from-the-kqueue-multipl.patch
Description: Binary data
v4-0002-oauth-Ensure-unused-socket-registrations-are-remo.patch
Description: Binary data
v4-0003-oauth-Remove-expired-timers-from-the-multiplexer.patch
Description: Binary data
v4-0004-oauth-Track-total-call-count-during-a-client-flow.patch
Description: Binary data
v4-0005-oauth-Add-unit-tests-for-multiplexer-handling.patch
Description: Binary data
