The thread model SERIALIZE_ALL_REQUESTS claims that although multiple handles may be open at the same time, "[...] data requests are serialized so that for the plugin as a whole only one read/write/etc request will be in progress at any time." The text seems to apply to only datapath requests but goes on to say: "This is a useful setting if the library you are using is not thread-safe. However performance may not be good." If the plugin calls into a non-thread-safe library during open() or close() (as the Python plugin does) then you are not protected. We could change every plugin, but it seems better to extend the protection of SERIALIZE_ALL_REQUESTS so it also applies to open() and close() callbacks, to reduce confusion. This fixes two problems in the Python plugin if the user-defined close() callback takes a long time to run (for example it is doing cleanups). See: https://bugzilla.redhat.com/show_bug.cgi?id=1566516 https://bugzilla.redhat.com/show_bug.cgi?id=1566522 --- docs/nbdkit-plugin.pod | 6 +++--- src/connections.c | 28 +++++++++++++++++----------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 6e89315..672f02a 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -663,9 +663,9 @@ first client to close the connection. I<This is a safe default for most plugins>. -Multiple handles can be open at the same time, but data requests are -serialized so that for the plugin as a whole only one read/write/etc -request will be in progress at any time. +Multiple handles can be open at the same time, but requests are +serialized so that for the plugin as a whole only one +open/read/write/close (etc) request will be in progress at any time. This is a useful setting if the library you are using is not thread-safe. However performance may not be good. diff --git a/src/connections.c b/src/connections.c index fbcf5e5..5024e2f 100644 --- a/src/connections.c +++ b/src/connections.c @@ -233,7 +233,7 @@ connection_worker (void *data) static int _handle_single_connection (int sockin, int sockout) { - int r = -1; + int ret = -1, r; struct connection *conn; int nworkers = threads ? threads : DEFAULT_PARALLEL_REQUESTS; pthread_t *workers = NULL; @@ -245,7 +245,10 @@ _handle_single_connection (int sockin, int sockout) if (!conn) goto done; - if (backend->open (backend, conn, readonly) == -1) + lock_request (conn); + r = backend->open (backend, conn, readonly); + unlock_request (conn); + if (r == -1) goto done; threadlocal_set_name (backend->plugin_name (backend)); @@ -312,11 +315,11 @@ _handle_single_connection (int sockin, int sockout) if (finalize (conn) == -1) goto done; - r = get_status (conn); + ret = get_status (conn); done: - debug ("connection cleanup with final status %d", r); + debug ("connection cleanup with final status %d", ret); free_connection (conn); - return r; + return ret; } int @@ -366,20 +369,23 @@ free_connection (struct connection *conn) conn->close (conn); - pthread_mutex_destroy (&conn->request_lock); - pthread_mutex_destroy (&conn->read_lock); - pthread_mutex_destroy (&conn->write_lock); - pthread_mutex_destroy (&conn->status_lock); - /* Don't call the plugin again if quit has been set because the main * thread will be in the process of unloading it. The plugin.unload * callback should always be called. */ if (!quit) { - if (conn->nr_handles > 0 && conn->handles[0]) + if (conn->nr_handles > 0 && conn->handles[0]) { + lock_request (conn); backend->close (backend, conn); + unlock_request (conn); + } } + pthread_mutex_destroy (&conn->request_lock); + pthread_mutex_destroy (&conn->read_lock); + pthread_mutex_destroy (&conn->write_lock); + pthread_mutex_destroy (&conn->status_lock); + free (conn->handles); free (conn); } -- 2.16.2