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

MainThreadMonitor: don't monitor thread if debugger is attached #2502

Merged
merged 2 commits into from May 18, 2023

Conversation

NachoSoto
Copy link
Contributor

See #2463.
If you're actively debugging and interrupt the main thread, it'll end up asserting, which isn't very convenient.

Copied the logic from https://stackoverflow.com/a/33177600/401024. It's not the safest code, but this only runs in tests.

See #2463.
If you're actively debugging and interrupt the main thread, it'll end up asserting, which isn't very convenient.

Copied the logic from https://stackoverflow.com/a/33177600/401024. It's not the safest code, but this only runs in tests.
@NachoSoto NachoSoto added the test Adding missing tests or correcting existing tests label May 17, 2023
@NachoSoto NachoSoto requested a review from a team May 17, 2023 21:15
@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #2502 (1838743) into main (64287a5) will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2502      +/-   ##
==========================================
+ Coverage   87.66%   87.72%   +0.05%     
==========================================
  Files         197      197              
  Lines       13347    13347              
==========================================
+ Hits        11700    11708       +8     
+ Misses       1647     1639       -8     

see 5 files with indirect coverage changes

// Counts buffer's size in bytes (like C/C++'s `sizeof`).
var size = MemoryLayout.stride(ofValue: info)
// Tells we want info about own process.
var mib: [Int32] = [CTL_KERN, KERN_PROC, KERN_PROC_PID, getpid()]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure but since this info isn't supposed to change, could mib be a let?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's passed as a reference in the next line so it can be modified. Swift would actually suggest this needs to be immutable otherwise :)

guard !Self.debuggerIsAttached else {
Logger.verbose("\(type(of: self)): debugger is attached, ignoring")
return
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when you run tests through Xcode, the debugger will be attached by default I think. I guess that means this monitor will only run when executing tests through the command line tool correct? We should just be aware that tests results may vary between executing them through xcode and through command line.

Copy link
Contributor Author

@NachoSoto NachoSoto May 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah or in CI. But you can also run them in Xcode without the debugger attached.

@NachoSoto NachoSoto merged commit 3ec75b1 into main May 18, 2023
14 checks passed
@NachoSoto NachoSoto deleted the main-thread-monitor-debugger branch May 18, 2023 15:07
NachoSoto added a commit that referenced this pull request May 25, 2023
)

See #2463.
If you're actively debugging and interrupt the main thread, it'll end up
asserting, which isn't very convenient.

Copied the logic from https://stackoverflow.com/a/33177600/401024. It's
not the safest code, but this only runs in tests.
This was referenced May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Adding missing tests or correcting existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants