oauth: Remove stale events from the kqueue multiplexer

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

diff --git a/src/interfaces/libpq-oauth/oauth-curl.c b/src/interfaces/libpq-oauth/oauth-curl.c
index dba9a684fa8a57daeb6907c4842991689c8ea958..433135cb86d59d94e4e9ec7379a1403b71a3be38 100644 (file)
--- a/src/interfaces/libpq-oauth/oauth-curl.c
+++ b/src/interfaces/libpq-oauth/oauth-curl.c
@@ -1376,6 +1376,53 @@ 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.
+ *
+ * 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)
+{
+#if defined(HAVE_SYS_EPOLL_H)
+       /* The epoll implementation doesn't hold onto stale events. */
+       return true;
+#elif defined(HAVE_SYS_EVENT_H)
+       struct timespec timeout = {0};
+       struct kevent ev;
+
+       /*
+        * Try to read a single pending event. We can actually ignore the result:
+        * either we found an event to process, in which case the multiplexer is
+        * correctly readable for that event at minimum, and it doesn't matter if
+        * there are any stale events; or we didn't find any, in which case the
+        * kernel will have discarded any stale events as it traveled to the end
+        * of the queue.
+        *
+        * Note that this depends on our registrations being level-triggered --
+        * even the timer, so we use a chained kqueue for that instead of an
+        * EVFILT_TIMER on the top-level mux. If we used edge-triggered events,
+        * this call would improperly discard them.
+        */
+       if (kevent(actx->mux, NULL, 0, &ev, 1, &timeout) < 0)
+       {
+               actx_error(actx, "could not comb kqueue: %m");
+               return false;
+       }
+
+       return true;
+#else
+#error comb_multiplexer is not implemented on this platform
+#endif
+}
+
 /*
  * Enables or disables the timer in the multiplexer set. The timeout value is
  * in milliseconds (negative values disable the timer).
@@ -2755,13 +2802,21 @@ pg_fe_run_oauth_flow_impl(PGconn *conn)
 
                                        if (status == PGRES_POLLING_FAILED)
                                                goto error_return;
-                                       else if (status != PGRES_POLLING_OK)
-                                       {
-                                               /* not done yet */
-                                               return status;
-                                       }
+                                       else if (status == PGRES_POLLING_OK)
+                                               break;  /* done! */
 
-                                       break;
+                                       /*
+                                        * This request is still running.
+                                        *
+                                        * 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
+                                        * signaled state and we'll burn CPU cycles pointlessly.
+                                        */
+                                       if (!comb_multiplexer(actx))
+                                               goto error_return;
+
+                                       return status;
                                }
 
                        case OAUTH_STEP_WAIT_INTERVAL: