From ebd979bbacbe56c105843186c57bd5dc64d6296d Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Wed, 7 Jun 2023 16:27:35 -0700 Subject: [PATCH] [SourceKit] Jump to a background queue before executing `SKDUIDFromUIdent` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `requestIsEnableBarriers` and `requestIsBarrier` call `SKDUIDFromUIdent`. What I didn’t know, was that `SKDUIDFromUIdent` can send XPC requests to the client to translate UIDs via `sourcekitd_set_uid_handlers`. Since we were calling `requestIsEnableBarriers` directly from the XPC server’s main queue, and the UID handler sends an XPC request synchronously, we could get into a deadlock situation. Immediately jump onto a serial background queue (`msgHandlingQueue`) in `sourcekitdServer_peer_event_handler` so that the main queue is free to execute the UID handler. This more closely matches the behavior before I introduced barriers as well, where we were always immediately jumping onto a concurrrent `msgHandlingQueue`. --- .../sourcekitd/bin/XPC/Service/XPCService.cpp | 108 +++++++++--------- 1 file changed, 56 insertions(+), 52 deletions(-) diff --git a/tools/SourceKit/tools/sourcekitd/bin/XPC/Service/XPCService.cpp b/tools/SourceKit/tools/sourcekitd/bin/XPC/Service/XPCService.cpp index a66d37644413d..7486a908ed927 100644 --- a/tools/SourceKit/tools/sourcekitd/bin/XPC/Service/XPCService.cpp +++ b/tools/SourceKit/tools/sourcekitd/bin/XPC/Service/XPCService.cpp @@ -232,6 +232,7 @@ static std::string getDiagnosticDocumentationPath() { } static dispatch_queue_t msgHandlingQueue; +static dispatch_queue_t requestQueue; static void sourcekitdServer_peer_event_handler(xpc_connection_t peer, xpc_object_t event) { @@ -256,66 +257,65 @@ static void sourcekitdServer_peer_event_handler(xpc_connection_t peer, assert(type == XPC_TYPE_DICTIONARY); // Handle the message xpc_retain(event); - if (xpc_object_t contents = xpc_dictionary_get_value(event, xpc::KeyMsg)) { - assert(xpc_get_type(contents) == XPC_TYPE_ARRAY); - sourcekitd_object_t req = xpc_array_get_value(contents, 0); - - void (^handler)(void) = ^{ - SourceKitCancellationToken cancelToken = - reinterpret_cast( - xpc_dictionary_get_uint64(event, xpc::KeyCancelToken)); - auto Responder = std::make_shared(event, peer); - xpc_release(event); - - sourcekitd::handleRequest(req, /*CancellationToken=*/cancelToken, - [Responder](sourcekitd_response_t response) { - Responder->sendReply(response); - }); - }; - - if (sourcekitd::requestIsEnableBarriers(req)) { - dispatch_barrier_async(msgHandlingQueue, ^{ + dispatch_async(msgHandlingQueue, ^{ + if (xpc_object_t contents = + xpc_dictionary_get_value(event, xpc::KeyMsg)) { + assert(xpc_get_type(contents) == XPC_TYPE_ARRAY); + sourcekitd_object_t req = xpc_array_get_value(contents, 0); + + void (^handler)(void) = ^{ + SourceKitCancellationToken cancelToken = + reinterpret_cast( + xpc_dictionary_get_uint64(event, xpc::KeyCancelToken)); auto Responder = std::make_shared(event, peer); xpc_release(event); - RequestBarriersEnabled = true; - sourcekitd::sendBarriersEnabledResponse([Responder](sourcekitd_response_t response) { - Responder->sendReply(response); + + sourcekitd::handleRequest(req, /*CancellationToken=*/cancelToken, + [Responder](sourcekitd_response_t response) { + Responder->sendReply(response); + }); + }; + + if (sourcekitd::requestIsEnableBarriers(req)) { + dispatch_barrier_async(requestQueue, ^{ + auto Responder = std::make_shared(event, peer); + xpc_release(event); + RequestBarriersEnabled = true; + sourcekitd::sendBarriersEnabledResponse([Responder](sourcekitd_response_t response) { + Responder->sendReply(response); + }); }); - }); - } else if (RequestBarriersEnabled && sourcekitd::requestIsBarrier(req)) { - dispatch_barrier_async(msgHandlingQueue, handler); - } else { - dispatch_async(msgHandlingQueue, handler); - } - } else if (xpc_object_t contents = - xpc_dictionary_get_value(event, "ping")) { - dispatch_async(msgHandlingQueue, ^{ + } else if (RequestBarriersEnabled && sourcekitd::requestIsBarrier(req)) { + dispatch_barrier_async(requestQueue, handler); + } else { + dispatch_async(requestQueue, handler); + } + } else if (xpc_object_t contents = + xpc_dictionary_get_value(event, "ping")) { // Ping back. xpc_object_t reply = xpc_dictionary_create_reply(event); xpc_release(event); assert(reply); xpc_connection_send_message(peer, reply); xpc_release(reply); - }); - } else if (SourceKitCancellationToken cancelToken = - reinterpret_cast( - xpc_dictionary_get_uint64(event, - xpc::KeyCancelRequest))) { - // Execute cancellation on a queue other than `msgHandling` so that we - // don’t block the cancellation of a request with a barrier - dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ - sourcekitd::cancelRequest(/*CancellationToken=*/cancelToken); - }); - } else if (SourceKitCancellationToken cancelToken = - reinterpret_cast( - xpc_dictionary_get_uint64( - event, xpc::KeyDisposeRequestHandle))) { - dispatch_async(msgHandlingQueue, ^{ + } else if (SourceKitCancellationToken cancelToken = + reinterpret_cast( + xpc_dictionary_get_uint64(event, + xpc::KeyCancelRequest))) { + // Execute cancellation on a queue other than `msgHandling` so that we + // don’t block the cancellation of a request with a barrier + dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ + sourcekitd::cancelRequest(/*CancellationToken=*/cancelToken); + }); + } else if (SourceKitCancellationToken cancelToken = + reinterpret_cast( + xpc_dictionary_get_uint64( + event, xpc::KeyDisposeRequestHandle))) { sourcekitd::disposeCancellationToken(/*CancellationToken=*/cancelToken); - }); - } else { - assert(false && "unexpected message"); - } + } else { + assert(false && "unexpected message"); + } + }); } } @@ -415,9 +415,13 @@ int main(int argc, const char *argv[]) { LOG_WARN_FUNC("getrlimit failed: " << llvm::sys::StrError()); } - auto attr = dispatch_queue_attr_make_with_qos_class(DISPATCH_QUEUE_CONCURRENT, + auto msgHandlingQueueAttr = dispatch_queue_attr_make_with_qos_class(DISPATCH_QUEUE_SERIAL, + QOS_CLASS_DEFAULT, 0); + msgHandlingQueue = dispatch_queue_create("message-handling", msgHandlingQueueAttr); + + auto requestQueueAttr = dispatch_queue_attr_make_with_qos_class(DISPATCH_QUEUE_CONCURRENT, QOS_CLASS_DEFAULT, 0); - msgHandlingQueue = dispatch_queue_create("request-handling", attr); + requestQueue = dispatch_queue_create("request-handling", requestQueueAttr); xpc_main(sourcekitdServer_event_handler); return 0;