Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

process monitor crashes #3010

Merged
merged 2 commits into from
Oct 5, 2024
Merged

process monitor crashes #3010

merged 2 commits into from
Oct 5, 2024

Conversation

n8henrie
Copy link
Member

@n8henrie n8henrie commented Jun 2, 2024

  • Simplify locks with @synchronized
  • Use a threadsafe dict for data as well as cache

@n8henrie
Copy link
Member Author

This is intended to fix #3008

@pjrobertson
Copy link
Member

Hey Nate 👋 :-)

Does this actually fix the problem? Have you been running with this and do you still experience crashes? If not - then it looks good to me. My only concern is: does it cause any locks anywhere else in the code?

@n8henrie n8henrie marked this pull request as draft September 23, 2024 02:58
@n8henrie
Copy link
Member Author

n8henrie commented Sep 23, 2024

Hi Pat!

Does this actually fix the problem?

No, unfortunately. I think I have mostly / partially reverted it in my local branch, will convert this to a draft to reduce confusion.

I've finally discovered xcode's thread sanitizer, which I think will be helpful in pinpointing areas of concern, but I've also been busy with a move and an upcoming board exam, so it's been slow going.

It also seem that thread sanitizer may have some false positives, as I've spent way too much time trying to hunt down the reported race condition at

NSIndexSet *matchedIndexes = [entries indexesOfObjectsWithOptions:NSEnumerationConcurrent passingTest:^BOOL(QSCatalogEntry *entry, NSUInteger idx, BOOL *stop) {
, but all I've figured out is that it goes away when I remove NSEnumerationConcurrent (and returns if I put it back in). Either that or it's somewhere in
- (NSArray *)contentsScanIfNeeded:(BOOL)canScan {
-- I think..

@pjrobertson
Copy link
Member

Hey hey!

Darn, that's a shame it doesn't work. Although I didn't think this would work - we used thread safe dicts in the past for QSProcessMonitor, but I actually took them out as they weren't helping :(

Good to hear you've got the thread sanitizer working! I wonder if ChatGPT can analyze and fix the code for us...? ;-)

@n8henrie
Copy link
Member Author

I wonder if ChatGPT can analyze and fix the code for us...?

I've tried! Unfortunately doesn't seem to know ObjC very well either. I tried (repeatedly) to get it to recreate a toy program that would crash with EXC_BAD_ACCESS due to a race condition, but the programs it generated all ran without errors or crashing.

@n8henrie n8henrie force-pushed the process-monitor-crashes branch from af772d5 to 363a4de Compare October 1, 2024 19:56
@n8henrie
Copy link
Member Author

n8henrie commented Oct 1, 2024

@pjrobertson actually, I take it back -- this seems to mitigate the issue at least somewhat. Without this I'm getting crashes within an hour or two, with this it's been running for 2 days without a crash. Let's merge this for now and see if it at least mitigates the issue.

@n8henrie n8henrie marked this pull request as ready for review October 1, 2024 19:58
@n8henrie n8henrie merged commit 133f01e into main Oct 5, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants