[sourcekit] Fix non-deterministic failure in CompileNotifications tests #17145
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci please smoke test |
|
Nitpick: typo in commit message ("deterinistic") |
|
@swift-ci please smoke test linux |
|
The linux failures seem to indicate we're losing this notification in some cases. It may be related to linux using the in-process sourcekitd. Investigating... |
This is a test-only change except for the introduction of a new request that is only used by tests. Our notifications are dispatched in the XPC event handler, which is not synchronized with replies to explicit XPC send_message_with_reply calls. This is fine for most users of sourcekitd, since the notifications are already enqueued on the client side, but for testing we need a way to guarantee that all notifications are passed to the client-side handler before we exit. This commit introduces a new request for testing that triggers a notification, allowing the client to wait on that notification to ensure all previously posted notifications have been handled. Note: the non-deterministic test failures can be triggered by adding a sleep of ~100 ms in the event handler before the notification is dispatched to the main queue. rdar://40311995
Setting the notification handler was racing with calling it; now both happen on the same queue. Only affected the in-process sourcekitd.
benlangmuir
force-pushed
the
notification-test-non-determinism
branch
from
507d508 to
8902562
Compare
|
Sure enough, there was a race in the in-process library (thanks TSan!). |
benlangmuir
changed the title
|
@swift-ci please smoke test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our notifications are dispatched in the XPC event handler, which is not
synchronized with replies to explicit XPC send_message_with_reply calls.
This is fine for most users of sourcekitd, since the notifications are
already enqueued on the client side, but for testing we need a way to
guarantee that all notifications are passed to the client-side handler
before we exit. This commit introduces a new request for testing that
triggers a notification, allowing the client to wait on that
notification to ensure all previously posted notifications have been
handled.
Note: the non-deterministic test failures can be triggered by adding a
sleep of ~100 ms in the event handler before the notification is
dispatched to the main queue.
Incidentally fix a race in the in-process sourcekit's notification handling
that became more obvious after this change.
rdar://40311995