[SR-1639] Move sourcekitdInProc to std::mutex

[modocache]

What's in this pull request?

SourceKit's in-process mode uses libdispatch sparingly. Removing all usages of libdispatch would allow SourceKit to more easily be ported to Linux. We can begin this process with the sourcekitd-test executable, which makes use of sourcekitd_send_request_sync.

When using an in-process model, sourcekitd::handleRequest is almost always synchronous, so we don't need to wait for the lambda to be executed. The only case in which it is not synchronous is for requests of type "sema", and only when the environment variable 'SOURCEKIT_DELAY_SEMA_EDITOR' is set. However, that feature deadlocks when using an in-process model anyway, so we don't need to worry about it here.

Resolved Related bug number: (SR-1639)

---

Before merging this pull request to apple/swift repository:

• Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

[gribozavr]

CC @akyrtzi @benlangmuir Could you review please?

[gribozavr]

@swift-ci Please smoke test OS X platform

[modocache]

Thanks! @benlangmuir and @akyrtzi, I also wrote up some context on all of these SourceKit changes as a comment on https://bugs.swift.org/browse/SR-710 -- hope that helps!

[benlangmuir]

@modocache this is not deadlocking; what you are seeing is that the sema request - in addition to returning a response - also sends a notification when semantic information is available. sourcekitd-test waits for that notification, which when you set SOURCEKIT_DELAY_SEMA_EDITOR will never come, because the semantic editor was disabled at the point that the request was made.

More importantly, there is at least one truly asynchronous call to handleRequest, which is cursor-info. So we really do need the semaphore in libsourcekitdInProc. I think a better approach would be to replace that with a more portable primitive like std::condition_variable.

What do you think?

[modocache]

@benlangmuir Excellent, thanks! I had initially replaced this with a condition variable, but wasn't sure how to make sure it was working. I searched around for something, but didn't find cursor-info. Thanks!!

By the way, how do you test that in-proc works? I noticed in-proc is used when building with ASAN... but when testing locally, I usually just comment that condition out. Is there a better way?

I'll move this over to condition_variable, and ping you when I do.

[benlangmuir]

I don't know that we explicitly test libsourcekitdInProc anywhere 😅. I use it as a debugging aide when there is a crash inside the service and I want to get the backtrace or debug it. We should theoretically re-run the SourceKit tests (or some fast subset of them) in-process, but in practice I don't think I've ever seen a bug that's specific to libsourcekitdInProc. For example the issue you mentioned here is actually not specific to in-process; you can reproduce it with the service by setting __XPC_SOURCEKIT_DELAY_SEMA_EDITOR=1 to forward the environment variable to the service.

[benlangmuir]

I'll move this over to condition_variable, and ping you when I do.

Great, thanks for working on this!

[modocache]

Awesome, thanks for your help, @benlangmuir! I managed to use -request=cursor to trigger an asynchronous code path. I tried removing the semaphore altogether, which (as I'd hoped) caused sourcekitd_send_request_sync to crash. I then added the std::condition_variable code in this new commit, and everything worked again.

The new commit message:

[SR-1639] Move sourcekitdInProc to std::mutex

SourceKit's in-process mode uses libdispatch sparingly. Removing all usages of
libdispatch would allow SourceKit to more easily be ported to Linux. We can
begin this process with the `sourcekitd-test` executable, which makes use of
`sourcekitd_send_request_sync`.

Migrate this usage to the C++ std::condition_variable.

As you mentioned, CI currently doesn't do anything with the in-process code, so CI has no chance of catching any logic errors. I'll trigger a build just to demonstrate it compiles, though. Let me know what you think!

@swift-ci Please smoke test OS X platform

[gribozavr]

@swift-ci Please smoke test Linux platform

[benlangmuir]

@modocache does this actually work when handleRequest is synchronous? Conditional variables usually need to be associated with some other variable ("the condition") to handle the case when they are signalled before you start waiting on them. I can't find anything that says C++ condition variable behaves any differently.

[gribozavr]

@modocache std::condition_variable allows for spurious wakeups, that's why you need a condition that you can reliably check.

[modocache]

Thanks, @benlangmuir and @gribozavr! I updated the pull request with a condition.

This is pretty much the second time I've written C++, and I learned two things:

1. The condition_variable behavior you both described. Thanks!
2. sourcekitd_response_t ReturnedResp does not initialize ReturnedResp to nullptr. I thought that it did. When I first wrote this code, I included the condition [&]{ return ReturnedResp != nullptr; }, but it always returned true immediately! Now I know. :)

I think it should work now! I tested it out with both -req=open and -req=cursor, both worked fine.

[benlangmuir]

Sadly because of spurious wakeup, this assignment now needs to be atomic or it can race with the read below.

[gribozavr]

The typical pattern is to use the same mutex to protect writing to the condition being waited on.

[modocache]

FYI, @briancroom has apparently met with some success in linking swift-corelibs-libdispatch to SourceKit. That would obviate the need to migrate code like this to std::condition_variable. I'll try to update this pull request such that it's technically correct based on @gribozavr's suggestion, but we probably shouldn't merge this just yet.

I think both @briancroom and I's work is based on #2704, #2763, and #2766, though. If someone could review/merge those first, that'd be great! 🙇

[modocache]

Closing this in favor of @briancroom's excellent work. Thanks all!