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
Fix: inotify read timeout not in ms #5876
Conversation
This was off by a factor of 1000, leading to a lot more invocations of the loop body than necessary.
Hello @grembo, thank you very much for submitting this PR to us! This is what will happen next:
Please allow up to 7 days for an initial review. We're all very excited about new pull requests but we only do this as a hobby. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #5876 +/- ##
=======================================
Coverage 96.90% 96.90%
=======================================
Files 405 405
Lines 16178 16179 +1
Branches 1170 1170
=======================================
+ Hits 15678 15679 +1
Misses 500 500
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Seems like a good catch to me, thanks. I'll let the person generally most versed in this side of things chime in before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, but lets tweak it a little bit
Lets calculate inotify_debounce * 1000
only once, and toss a _ms
so no one forgets this value is in milliseconds
df5e724
to
76a6cc6
Compare
76a6cc6
to
d2cf1b8
Compare
@stumpylog Do you mean something like this? Note that we still need a seconds representation of inotify_debounce within the loop body - therefore my solution would have been to rename it to inotify_debounce_secs (like I did), but keep the multiplication * 1000 - this iteration introduces the extra variable like requested (and also renames timeout to timeout_ms to make this explicit), like this: timeout_ms = inotify_debounce_secs * 1000 There are certainly many different ways to resolve this ^_^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns. See our contributing guidelines for more details. |
This was off by a factor of 1000, leading to a lot more invocations of the loop body than necessary.
Proposed change
Calculate the timeout passed to inotify.read to be in milliseconds (as required by the function) instead of seconds.
The issue becomes easily perceivable when setting
PAPERLESS_CONSUMER_INOTIFY_DELAY
to a high number, so that the while loop becomes obvious in tools like top.Type of change
Checklist:
pre-commit
hooks, see documentation.