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

Add method for native watcher initialization with fallback #441

Closed
wants to merge 3 commits into from

Conversation

0xpr03
Copy link
Member

@0xpr03 0xpr03 commented Aug 31, 2022

Ads a new method for initializing the recommended watcher and falling back to pollwatcher on failure to do so

Fixes #423

@0xpr03
Copy link
Member Author

0xpr03 commented Aug 31, 2022

This is a draft because:

  1. I'd like to also change the recommended_watcher function to just call the new one, giving it more value, but would have to change its result value. Not sure if we can do that semver compatible (or just ignore it).
  2. I've opted for an enum type instead of Box<dyn Watcher>, I could also imagine using that.
  3. I've only handled the specific Function not implemented (os error 38) when running on docker in M1 MacOS  #423 error. We could also fallback for any error.
  4. Do we want to report the original error with the fallback ? Add some additional error kind "unavailable API" to at least report it?
  5. This requires EventHandler to be declared either Copy or Clone for this fallback function. Even more of a SemVer break for recommended_watcher

Todo:

  • Add docs entry in the issue for using that
  • Change example to use the new function
  • allow debouncer init with fallback (maybe add an easier init)

CC @samuelcolvin as possible consumer

@0xpr03 0xpr03 requested a review from JohnTitor August 31, 2022 15:30
@0xpr03 0xpr03 force-pushed the fixup_423 branch 3 times, most recently from 4fad052 to d539e4c Compare August 31, 2022 15:59
notify/src/lib.rs Outdated Show resolved Hide resolved
@JohnTitor
Copy link
Member

JohnTitor commented Sep 2, 2022

  1. I'd like to also change the recommended_watcher function to just call the new one, giving it more value, but would have to change its result value. Not sure if we can do that semver compatible (or just ignore it).

I'd call it a breaking change if it changes behavior.

  1. I've opted for an enum type instead of Box<dyn Watcher>, I could also imagine using that.

I think it's totally fine while we have take_boxed.

  1. I've only handled the specific Function not implemented (os error 38) when running on docker in M1 MacOS #423 error. We could also fallback for any error.

These could come later :)

  1. Do we want to report the original error with the fallback ? Add some additional error kind "unavailable API" to at least report it?

I don't have a strong opinion on that, maybe we could prepare an optional value?

  1. This requires EventHandler to be declared either Copy or Clone for this fallback function. Even more of a SemVer break for recommended_watcher

Hmm, that sounds like a rather difficult issue and I cannot prepare a solution for that soon (I'm now fixing my dev env and cannot check your code deeply).

Copy link
Contributor

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

broadly seems fine to me, I'll probably stick with that I have now in watchfiles though since it's working well.

#[allow(unused)]
ErrorKind::Io(io_error) => {
#[cfg(target_os = "linux")]
if io_error.raw_os_error() == Some(38) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a documented meaning to 38 or indeed other codes somewhere? I suspect we should fallback in more cases but I have no idea how to find out which ones should be an error and which should fallback.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, here is the manpage though I prefer this list
Problem is that this does not tell us which of these could be emitted by our specific syscall.


impl WatcherFallback {
/// Returns the watcher inside
pub fn take_boxed(self) -> Box<dyn Watcher>{
Copy link
Contributor

Choose a reason for hiding this comment

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

personally I would prefer that this implemented watch. Is that very verbose/difficult?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's a very good idea! and it would make the enum more worthwhile (because otherwise I don't actually see much of a value over directly boxing it, except for claiming the allocation-free throne, while not being useful at all)

Copy link
Member Author

@0xpr03 0xpr03 Sep 2, 2022

Choose a reason for hiding this comment

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

Ok I tried this.. And it gets awkward.

  • We can't implement Watcher::new due to missing Copy/Clone on event_handler.
  • We can't implement a real kind(), as we don't have access to self. This kind of API is specifically not designed for runtime-selected watchers it seems. (V6 incoming.. ) We could still implement the functions without the trait, or leave them unimplemented!() for the new and the additional watcher-kind as in my new code.

Edit: But we can implement Deref and DerefMut, allowing direct Watcher::* access, but also kind(&self), which isn't the Trait, but at least gives the functionality.

@0xpr03
Copy link
Member Author

0xpr03 commented Sep 4, 2022

Please review again, the addition of Deref/DerefMut allows direct access to the watcher in our enum, making the whole stack allocation worthwhile, such that boxing is not required. Example

Open Questions:

  • Do we deprecate recommended_watcher() to reflect that it's kind of unhelpful as a function (in light of RecommendedWatcher) ?
  • Do we migrate all examples (except for one) and the docs to recommend using recommended_watcher_fallback over RecommendedWatcher ? Technically it is the only way which we can get to actually work on all systems (and can work around future issues). In a way comptime-selection was fun while it lasted. On the other hand a lot of people don't seem to care about the specific issue on M1 + Docker (you can also say people should not expect such weird emulator setups to work), but I don't see a drawback for people already using RecommendedWatcher to migrate to the dynamic approach.
  • Bikeshed: Is there a better name than recommended_watcher_fallback ? It's kinda long when for normal users I'd say it is the recommended way.
  • I have no good answer to the copy/clone requirement of the fallback logic. We need it for recommended_watcher_fallback, so it's an additional requirement. We may just leave it as that.

@juliusl
Copy link

juliusl commented Oct 12, 2022

Bikeshed: Is there a better name than recommended_watcher_fallback ? It's kinda long when for normal users I'd say it is the recommended way. @0xpr03

How about ensure_watcher? From my experience, ensure has a stronger implication that a resource will be obtained.

@0xpr03 0xpr03 closed this Aug 16, 2023
@0xpr03 0xpr03 deleted the fixup_423 branch August 16, 2023 22:03
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.

Function not implemented (os error 38) when running on docker in M1 MacOS
4 participants