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

Avoid panic when using a released CFRunLoop #39

Closed
wants to merge 1 commit into from

Conversation

adriansr
Copy link
Contributor

As fsevents is saving the result of CFRunLoopGetCurrent and stopping
it from a separate thread, it's necessary to increase the reference
count of the returned CFRunLoopRef.

In the event that the run loop is terminated by other means, stop
might be working with a deallocated reference and produce a panic
(SIGSEGV).

What does this pull request do?

Fixes an occassional panic

Where should the reviewer start?

How should this be manually tested?

As fsevents is saving the result of `CFRunLoopGetCurrent` and stopping
it from a separate thread, it's necessary to increase the reference
count of the returned CFRunLoopRef.

In the event that the run loop is terminated by other means, `stop`
might be working with a deallocated reference and produce a panic
(SIGSEGV).
@codecov-io
Copy link

codecov-io commented Jul 18, 2018

Codecov Report

Merging #39 into master will increase coverage by 0.19%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
+ Coverage   71.18%   71.37%   +0.19%     
==========================================
  Files           3        3              
  Lines         288      290       +2     
==========================================
+ Hits          205      207       +2     
  Misses         61       61              
  Partials       22       22
Impacted Files Coverage Δ
wrap.go 70.54% <100%> (+0.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed9a50f...7304cd6. Read the comment docs.

@nathany
Copy link
Contributor

nathany commented Aug 26, 2018

@adriansr Thanks for the patch. @patsoffice Thanks for reviewing.

Will merge this after #41 is merged (which fixes failing tests in Go 1.10.4 and Go 1.11).

@andrewkroh
Copy link

@nathany Given that #41 is merged can this PR be merged? Thanks.

@nathany nathany closed this in 972ac2c Jan 5, 2019
@nathany
Copy link
Contributor

nathany commented Jan 5, 2019

@adriansr @andrewkroh Yes, apologies for the delay. Merged.

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.

5 participants