Skip to content

Address clippy lints, hide some fsevents functions#312

Merged
0xpr03 merged 7 commits intonotify-rs:mainfrom
erickt:clippy
May 13, 2021
Merged

Address clippy lints, hide some fsevents functions#312
0xpr03 merged 7 commits intonotify-rs:mainfrom
erickt:clippy

Conversation

@erickt
Copy link
Contributor

@erickt erickt commented May 12, 2021

This addresses all the items found by clippy 0.1.54 (ca82264e 2021-05-09). In addition, it hides the functions fsevents::Callback and fsevents::CFRunLoopIsWaiting, which I don't think were intended to be made public.

@0xpr03
Copy link
Member

0xpr03 commented May 12, 2021

I'm not sure why the macos-latest run fails only for this PR with a timeout after 46minutes. I retried the CI and it failed again.

erickt added 5 commits May 12, 2021 11:40
This avoids accidentally including non-hermetic data in tests.
While I couldn't prove any concrete issues before this patch, it can be risky
to have both the event stream thread and the main thread access the context
at the same time, since it could lead to data races or undefined behavior.
This changes the handling of the context to factor out the `done` channel
from the context, and to move explicitly dropping the context once the
stream has been stopped.

Furthermore, this creates some helper wrappers to send CFRef types across
threads instead of casting values to integers.
@erickt
Copy link
Contributor Author

erickt commented May 12, 2021

I had a bad cast in the callback, which I've fixed. I've added two more fix:

  • I found and fixed a memory leak (found with RUSTFLAGS=-Zsanitizer=leak cargo test --target x86_64-apple-darwin).
  • I rewrote how CFRefs were being passed between threads in order to avoid casts, and moved freeing the context into the thread after the thread was shut down to avoid the risk that we could add a path that freed the context out from under the event stream.

@0xpr03
Copy link
Member

0xpr03 commented May 12, 2021

Thanks for all the work, this looks pretty well. I think we are really in need of somebody that has a mac. Do you want this merged now or are you planning to add more ?

Also please also add a changelog entry like previously for the "unreleased" section with a link to your PR. Or just leave a note and I'll do that.

@0xpr03
Copy link
Member

0xpr03 commented May 12, 2021

Also I'll have to check, the public stuff could be from the deleted tests we've had previously.

@erickt
Copy link
Contributor Author

erickt commented May 13, 2021

@0xpr03 - thanks! added a CHANGELOG comment. I think this PR is ready to go. I'll rebase #313 on top of this once it lands.

@0xpr03 0xpr03 merged commit 5e1b69a into notify-rs:main May 13, 2021
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