Author: brane
Date: Fri Jul 25 02:22:06 2025
New Revision: 1927456
Log:
Get rid of the mutex that serializes access to the async resolver results.
They're a singly-linked stack, so use lockless push and clear instead.
* serf_private.h
(serf_context_t): Make the resolve_head volatile, and just a void*.
Remove resolve_guard and all the #if thread stuff.
* src/context.c
(serf_context_create_ex): Simplify the initialization for async resolving.
* src/resolve.c: Include <apr_version.h>. Update the top-level todo list.
(lock_results, unlock_results): Remove.
(apr_atomic_casptr, apr_atomic_xchgptr): Add wrapper macros for APR-1.x,
because the prototypes in that version are wrong.
(push_resolve_result): Use a lockless push to the result stack.
(serf__process_async_resolve_results): No locking, just a pointer exchange.
Modified:
serf/trunk/serf_private.h
serf/trunk/src/context.c
serf/trunk/src/resolve.c
Modified: serf/trunk/serf_private.h
==============================================================================
--- serf/trunk/serf_private.h Fri Jul 25 01:26:09 2025 (r1927455)
+++ serf/trunk/serf_private.h Fri Jul 25 02:22:06 2025 (r1927456)
@@ -499,11 +499,8 @@ struct serf_context_t {
serf_config_t *config;
/* Support for asynchronous address resolution. */
+ void *volatile resolve_head;
apr_status_t resolve_init_status;
- serf__resolve_result_t *resolve_head;
-#if APR_HAS_THREADS
- apr_thread_mutex_t *resolve_guard;
-#endif
void *resolve_context;
};
Modified: serf/trunk/src/context.c
==============================================================================
--- serf/trunk/src/context.c Fri Jul 25 01:26:09 2025 (r1927455)
+++ serf/trunk/src/context.c Fri Jul 25 02:22:06 2025 (r1927456)
@@ -195,18 +195,9 @@ serf_context_t *serf_context_create_ex(
ctx->server_authn_info = apr_hash_make(pool);
/* Initialize async resolver result queue. */
- ctx->resolve_init_status = APR_SUCCESS;
ctx->resolve_head = NULL;
-#if APR_HAS_THREADS
- ctx->resolve_init_status = apr_thread_mutex_create(
- &ctx->resolve_guard, APR_THREAD_MUTEX_DEFAULT, ctx->pool);
- if (ctx->resolve_init_status != APR_SUCCESS) {
- ctx->resolve_guard = NULL;
- }
-#endif
- if (ctx->resolve_init_status == APR_SUCCESS) {
- ctx->resolve_init_status = serf__create_resolve_context(ctx);
- }
+ ctx->resolve_init_status = APR_SUCCESS;
+ ctx->resolve_init_status = serf__create_resolve_context(ctx);
if (ctx->resolve_init_status != APR_SUCCESS) {
ctx->resolve_context = NULL;
}
Modified: serf/trunk/src/resolve.c
==============================================================================
--- serf/trunk/src/resolve.c Fri Jul 25 01:26:09 2025 (r1927455)
+++ serf/trunk/src/resolve.c Fri Jul 25 02:22:06 2025 (r1927456)
@@ -19,6 +19,7 @@
*/
#include <apr.h>
+#include <apr_version.h>
/* Include the headers needed for inet_ntop and related structs.
On Windows, we'll always get <Winsock2.h> from <apr.h>. */
@@ -59,12 +60,6 @@
* - Wake the poll/select in serf_context_run() when new resolve
* results are available.
*
- * - Figure out what to do if the lock/unlock calls return an error.
- * This should not be possible unless we messed up the implementation,
- * but there should be a way for clients to back out of this situation.
- * Failed lock/unlock could potentially leave the context in an
- * inconsistent state.
- *
* TODO for Unbound:
* - Convert unbound results to apr_sockaddr_t.
*/
@@ -650,39 +645,15 @@ static apr_status_t run_async_resolver_l
/* The result queue implementation. */
#if HAVE_ASYNC_RESOLVER
-static apr_status_t lock_results(serf_context_t *ctx)
-{
-#if APR_HAS_THREADS
- apr_status_t status = apr_thread_mutex_lock(ctx->resolve_guard);
- if (status) {
- /* TODO: ctx->error_callback... */
- char buffer[256];
- serf__log(LOGLVL_ERROR, LOGCOMP_CONN, __FILE__, ctx->config,
- "Lock async resolve results: %s\n",
- apr_strerror(status, buffer, sizeof(buffer)));
- }
- return status;
-#else
- return APR_SUCCESS;
-#endif
-}
-
-static apr_status_t unlock_results(serf_context_t *ctx)
-{
-#if APR_HAS_THREADS
- apr_status_t status = apr_thread_mutex_unlock(ctx->resolve_guard);
- if (status) {
- /* TODO: ctx->error_callback... */
- char buffer[256];
- serf__log(LOGLVL_ERROR, LOGCOMP_CONN, __FILE__, ctx->config,
- "Unlock async resolve results: %s\n",
- apr_strerror(status, buffer, sizeof(buffer)));
- }
- return status;
-#else
- return APR_SUCCESS;
-#endif
-}
+#if APR_MAJOR_VERSION < 2
+/* NOTE: The atomic pointer prototypes in apr-1.x are just horribly
+ wrong. They're fixed in version 2.x and these macros deal
+ with the difference. */
+#define apr_atomic_casptr(mem, with, cmp) \
+ (apr_atomic_casptr)((volatile void**)(mem), (with), (cmp))
+#define apr_atomic_xchgptr(mem, with) \
+ (apr_atomic_xchgptr)((volatile void**)(mem), (with))
+#endif /* APR_MAJOR_VERSION < 2 */
static void push_resolve_result(serf_context_t *ctx,
@@ -693,7 +664,7 @@ static void push_resolve_result(serf_con
apr_pool_t *resolve_pool)
{
serf__resolve_result_t *result;
- apr_status_t status;
+ void *head;
result = apr_palloc(resolve_pool, sizeof(*result));
result->host_address = host_address;
@@ -702,15 +673,15 @@ static void push_resolve_result(serf_con
result->resolved_baton = resolved_baton;
result->result_pool = resolve_pool;
- status = lock_results(ctx);
- if (!status)
- {
- result->next = ctx->resolve_head;
- ctx->resolve_head = result;
- status = unlock_results(ctx);
- }
-
- /* TODO: if (status) ... then what? */
+ /* Atomic push this result to the result stack. This might look like
+ a potential priority inversion, however, it's not likely that we'll
+ resolve several tens of thousands of results per second in hundreds
+ of separate threads. */
+ head = apr_atomic_casptr(&ctx->resolve_head, NULL, NULL);
+ do {
+ result->next = head;
+ head = apr_atomic_casptr(&ctx->resolve_head, result, head);
+ } while(head != result->next);
}
@@ -724,25 +695,15 @@ apr_status_t serf__create_resolve_contex
/* Internal API */
apr_status_t serf__process_async_resolve_results(serf_context_t *ctx)
{
- serf__resolve_result_t *result = NULL;
+ serf__resolve_result_t *result;
apr_status_t status;
status = run_async_resolver_loop(ctx);
if (status)
return status;
- status = lock_results(ctx);
- if (status)
- return status;
-
- result = ctx->resolve_head;
- ctx->resolve_head = NULL;
- status = unlock_results(ctx);
-
- /* TODO: if (status) ... then what? Shouldn't be possible. */
- /* if (status) */
- /* return status; */
-
+ /* Grab the whole stack, leaving it empty, and process the contents. */
+ result = apr_atomic_xchgptr(&ctx->resolve_head, NULL);
while (result)
{
serf__resolve_result_t *const next = result->next;
