Skip to content

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

Merged
NachoSoto merged 2 commits into
mainfrom
main-thread-monitor-debugger
May 18, 2023
Merged

MainThreadMonitor: don't monitor thread if debugger is attached#2502
NachoSoto merged 2 commits into
mainfrom
main-thread-monitor-debugger

Conversation

@NachoSoto

Copy link
Copy Markdown
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 label May 17, 2023
@NachoSoto NachoSoto requested a review from a team May 17, 2023 21:15
@codecov

codecov Bot commented May 17, 2023

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@NachoSoto NachoSoto May 18, 2023

Copy link
Copy Markdown
Contributor Author

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
@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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants