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

issue #509: raise Listen::Error::INotifyMaxWatchesExceeded rather than abort #545

Merged
merged 2 commits into from Aug 19, 2021

Conversation

ColinDKelley
Copy link
Collaborator

Fix for issue #509:

  • When iNotify max watchers exceeded, raise INotifyMaxWatchesExceeded rather than abort.

@ColinDKelley ColinDKelley force-pushed the issue#509/raise-rather-than-abort branch from 12c3273 to 5ae7b48 Compare August 15, 2021 06:08
@ColinDKelley ColinDKelley linked an issue Aug 15, 2021 that may be closed by this pull request
@ColinDKelley ColinDKelley added the 🐛 Bug Fix Fixes a bug label Aug 15, 2021
@ColinDKelley ColinDKelley removed the 🐛 Bug Fix Fixes a bug label Aug 15, 2021
@ColinDKelley ColinDKelley changed the base branch from master to v3.0 August 15, 2021 10:46
@ColinDKelley ColinDKelley changed the base branch from v3.0 to master August 15, 2021 10:46
@ColinDKelley ColinDKelley force-pushed the issue#509/raise-rather-than-abort branch from 5ae7b48 to a678b9e Compare August 15, 2021 10:47
@ColinDKelley
Copy link
Collaborator Author

BTW, I'm expecting to release this in v3.7.0 in the coming week.

@ColinDKelley ColinDKelley force-pushed the issue#509/raise-rather-than-abort branch from a678b9e to 2972907 Compare August 18, 2021 02:39
@ColinDKelley ColinDKelley requested review from ioquatix and removed request for ioquatix August 18, 2021 02:40
Copy link
Member

@ioquatix ioquatix left a comment

Choose a reason for hiding this comment

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

It looks okay to me. However, I'm not sure if the exception message needs to be so elaborate?

@ColinDKelley
Copy link
Collaborator Author

ColinDKelley commented Aug 18, 2021

@ioquatix I carried the exception across from the old warning message. I like that it points to the documentation.
How would you simplify it? Were you thinking of putting it all on one line maybe? And maybe drop some superfluous words like FATAL and error?
How about:

Listen gem unable to monitor directories for changes because iNotify max watches exceeded. See: #{README_URL}

@ColinDKelley ColinDKelley force-pushed the issue#509/raise-rather-than-abort branch from 254dff9 to 9c7a4a7 Compare August 18, 2021 05:20
@ioquatix
Copy link
Member

I think the exception message shouldn't contain documentation. So, I'd just say "Max watches exceeded!".

Personally, I don't mind either approach, but if you start adding URLs to exception messages, surely that should apply to all exceptions? So, it's hard to know where to draw the line. Perhaps having documentation on the exception class which explains what to do to mitigate it.

@ColinDKelley
Copy link
Collaborator Author

ColinDKelley commented Aug 19, 2021

I don't see any downside to the exception including a link to documentation on how to resolve the problem! The reason we don't do it all the time is that it's a lot of work. ;) In this case, someone before me already did it, and I confirmed that the link still works (since I refactored it into the README a couple months ago) so I think we're good. I will trim it down to the more succinct version, though.

@ColinDKelley ColinDKelley merged commit acabb6d into guard:master Aug 19, 2021
@ColinDKelley ColinDKelley deleted the issue#509/raise-rather-than-abort branch August 19, 2021 05:28
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.

linux driver can abort the enclosing process
2 participants