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

Simpify System.FSNotify.readEvents #43

Closed
feuerbach opened this Issue Dec 4, 2013 · 6 comments

Comments

Projects
None yet
3 participants
@feuerbach
Member

feuerbach commented Dec 4, 2013

readEvents does some locking. The purpose of that seems to be to disallow multiple callbacks to overlap.

Is that necessary? It seems to me like a user's responsibility.

@gregwebs

This comment has been minimized.

Show comment
Hide comment
@gregwebs

gregwebs Dec 4, 2013

Member

Yes, I believe the intent is for only one callback should be operating at a time. Consider a rule to re-compile a Haskell project when a file is changed. It would be bad form to attempt 3 re-compiles at once if a file was rapidly saved 3 times. Better would be to wait for the first re-compile to finish.

With the existing API I think the locking can be avoided by putting forkIO in the callback. Perhaps we need to provide a convenient way for the user to specify whether locking is desired. Or we could provide a helper function so that the user can add locking to their callback.

I actually see 3 use cases here:

  • no locking
  • per path action locking
  • action locking
Member

gregwebs commented Dec 4, 2013

Yes, I believe the intent is for only one callback should be operating at a time. Consider a rule to re-compile a Haskell project when a file is changed. It would be bad form to attempt 3 re-compiles at once if a file was rapidly saved 3 times. Better would be to wait for the first re-compile to finish.

With the existing API I think the locking can be avoided by putting forkIO in the callback. Perhaps we need to provide a convenient way for the user to specify whether locking is desired. Or we could provide a helper function so that the user can add locking to their callback.

I actually see 3 use cases here:

  • no locking
  • per path action locking
  • action locking
@feuerbach

This comment has been minimized.

Show comment
Hide comment
@feuerbach

feuerbach Dec 4, 2013

Member

Consider a rule to re-compile a Haskell project when a file is changed. It would be bad form to attempt 3 re-compiles at once if a file was rapidly saved 3 times. Better would be to wait for the first re-compile to finish.

Sure, but that shouldn't be the concern of the library. In some cases locking is desired, in some it isn't, in some it's even insufficient. E.g. in my application no two actions should overlap even for distinct paths.

My main concern is the code complexity that grows from this. First we have some code doing locking in the library, then in the user code we have some additional code that either works around locking or implements a different locking scheme.

So I propose to implement the simplest scheme possible (and also the one that I, as a user, would expect from such a library) — which is actions are executed asynchronously in response to events.

It is already straightforward to implement locking on the user side if necessary.

Member

feuerbach commented Dec 4, 2013

Consider a rule to re-compile a Haskell project when a file is changed. It would be bad form to attempt 3 re-compiles at once if a file was rapidly saved 3 times. Better would be to wait for the first re-compile to finish.

Sure, but that shouldn't be the concern of the library. In some cases locking is desired, in some it isn't, in some it's even insufficient. E.g. in my application no two actions should overlap even for distinct paths.

My main concern is the code complexity that grows from this. First we have some code doing locking in the library, then in the user code we have some additional code that either works around locking or implements a different locking scheme.

So I propose to implement the simplest scheme possible (and also the one that I, as a user, would expect from such a library) — which is actions are executed asynchronously in response to events.

It is already straightforward to implement locking on the user side if necessary.

@gregwebs

This comment has been minimized.

Show comment
Hide comment
@gregwebs

gregwebs Dec 4, 2013

Member

ok, I agree. But we should provide helper functions for the locking strategies so that every user doesn't have to re-implement it. Perhaps those would go in the Devel module which is poorly named.

Member

gregwebs commented Dec 4, 2013

ok, I agree. But we should provide helper functions for the locking strategies so that every user doesn't have to re-implement it. Perhaps those would go in the Devel module which is poorly named.

@gregwebs

This comment has been minimized.

Show comment
Hide comment
@gregwebs

gregwebs Dec 4, 2013

Member

I do think action locking is pretty essential though to the point that we might want to build it into the Action type so that the user has to declare their strategy.

Member

gregwebs commented Dec 4, 2013

I do think action locking is pretty essential though to the point that we might want to build it into the Action type so that the user has to declare their strategy.

@feuerbach

This comment has been minimized.

Show comment
Hide comment
@feuerbach

feuerbach Dec 4, 2013

Member

Ok, I think it's doable.

Member

feuerbach commented Dec 4, 2013

Ok, I think it's doable.

feuerbach added a commit that referenced this issue Dec 4, 2013

feuerbach added a commit that referenced this issue Dec 4, 2013

@thomasjm

This comment has been minimized.

Show comment
Hide comment
@thomasjm

thomasjm May 29, 2018

Contributor

Cleaning up old issues now. For this one, given that it only takes a few lines of code for a user to make MVar(s) and do withMVar on them, I don't see what kind of helpers are envisioned here. I don't think it's worth the complexity to add locking helpers here, since simple ones are easy for the user to do on their own and more elaborate ones involve trying to anticipate more elaborate locking policies. (The issue is also entangled with debouncing.) I'm going to hit close on this issue, feel free to ping back if anyone disagrees and we can revisit.

Contributor

thomasjm commented May 29, 2018

Cleaning up old issues now. For this one, given that it only takes a few lines of code for a user to make MVar(s) and do withMVar on them, I don't see what kind of helpers are envisioned here. I don't think it's worth the complexity to add locking helpers here, since simple ones are easy for the user to do on their own and more elaborate ones involve trying to anticipate more elaborate locking policies. (The issue is also entangled with debouncing.) I'm going to hit close on this issue, feel free to ping back if anyone disagrees and we can revisit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment