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

No error when stopping watching a non watched file #455

Closed
wants to merge 1 commit into from

Conversation

MidasLamb
Copy link

What does this pull request do?

Currently, when a non-watched item is requested to be removed from a
watch, an error is returned.
However, the post-condition of removing it is: "This file/dir should not
be watched anymore", which is the case also when the item wasn't being
watched in the first place.
Rather than returning an error. Return nil and skip a part of the work.

Where should the reviewer start?

inotify.go, the Remove function definition.

How should this be manually tested?

We've noticed this because this lib is being used in promtail and it was logging errors because of this.

Currently, when a non-watched item is requested to be removed from a
watch, an error is returned.
However, the post-condition of removing it is: "This file/dir should not
be watched anymore", which is the case also when the item wasn't being
watched in the first place.
Rather than returning an error. Return nil and skip a part of the work.
@mattn
Copy link
Member

mattn commented Jun 3, 2022

If this Remove function returns a wrapped error for os.ErrNotExist, will your problem be resolved with using erros.Is?

@MidasLamb
Copy link
Author

If this Remove function returns a wrapped error for os.ErrNotExist, will your problem be resolved with using erros.Is?

That could also be a possibility. I'm not that experienced in go so I don't know what would be the most idiomatic way.
I can update the PR if you think it's a better approach

@mattn
Copy link
Member

mattn commented Jun 3, 2022

I prefer to add ErrNonExistentWatch error in fsnotify.go

var (
    ErrNonExistentWatch = errors.New("can't remove non-existent watcher")
    ErrEventOverflow    = errors.New("fsnotify queue overflow")
)

and return it in inotify.go, kqueue.go, windows.go

return fmt.Errorf("%w: %s", ErrNonExistentWatch, name)

any thought? @nathany @shogo82148

@nathany
Copy link
Contributor

nathany commented Jun 16, 2022

@mattn a new error sounds good to me

arp242 pushed a commit that referenced this pull request Jul 21, 2022
The errors returned by the various implementations of the watcher are all different
which makes handling them difficult. This PR follows the suggestion in:
#455 (comment) by @mattn
to create a common error which is wrapped by the implementations.

Replaces: #455
Signed-off-by: Andrew Thornton <art27@cantab.net>
@arp242
Copy link
Member

arp242 commented Jul 21, 2022

I agree that outright removing the error is not a good solution; I opened #460 to return a specific error that can be tested for, which should address the issue better.

@arp242 arp242 closed this Jul 21, 2022
arp242 added a commit that referenced this pull request Jul 21, 2022
The errors returned by the various implementations of the watcher are all different
which makes handling them difficult. This PR follows the suggestion in:
#455 (comment) by @mattn
to create a common error which is wrapped by the implementations.

Replaces: #455
Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: Andrew Thornton <art27@cantab.net>
shogo82148 pushed a commit to shogo82148/fsnotify that referenced this pull request Mar 6, 2024
port of fsnotify/fsnotify#460

The errors returned by the various implementations of the watcher are all different
which makes handling them difficult. This PR follows the suggestion in:
fsnotify/fsnotify#455 (comment) by @mattn
to create a common error which is wrapped by the implementations.
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.

None yet

4 participants