Refactor(watcher): replace Queue with Semaphore #1118
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dairiki
force-pushed
the
fix.watcher-fileopened-events
branch
from
1d790bb
to
450b56b
Compare
dairiki
force-pushed
the
refactor.watcher
branch
from
e1bb0d6
to
6855ec9
Compare
dairiki
changed the title
dairiki
force-pushed
the
fix.watcher-fileopened-events
branch
from
450b56b
to
474fd20
Compare
dairiki
force-pushed
the
refactor.watcher
branch
2 times, most recently
from
5084991
to
63e35b7
Compare
dairiki
force-pushed
the
fix.watcher-fileopened-events
branch
2 times, most recently
from
2276354
to
db85427
Compare
dairiki
force-pushed
the
refactor.watcher
branch
from
63e35b7
to
a0fc8db
Compare
2 tasks
dairiki
force-pushed
the
fix.watcher-fileopened-events
branch
from
db85427
to
9ced7ef
Compare
dairiki
force-pushed
the
refactor.watcher
branch
2 times, most recently
from
960e24e
to
1e8c7ee
Compare
The current implementation stores all unprocessed filesystem change events in an unbounded Queue. There can possibly be quite a number of events, so this can be (in some situations) a memory consumption issue. All we really care is whether there has been *any* interesting event since the last time we checked. All we need for that is a flag, not a queue.
This avoid a tiny (unlikely) race condition with the Event strategy.
Remove the unused `time` and `event_type` parameters from `Watcher.is_interesting()`.
dairiki
force-pushed
the
refactor.watcher
branch
from
1e8c7ee
to
c3214cf
Compare
dairiki
force-pushed
the
refactor.watcher
branch
from
c3214cf
to
a7bc589
Compare
dairiki
force-pushed
the
refactor.watcher
branch
from
c3214cf
to
685fa05
Compare
dairiki
added a commit
to dairiki/lektor
that referenced
this pull request
Sep 11, 2023
* refactor(watcher): replace Queue with flag (threading.BoundedSemaphore) The current implementation stores all unprocessed filesystem change events in an unbounded Queue. There can possibly be quite a number of events, so this can be (in some situations) a memory consumption issue. All we really care is whether there has been *any* interesting event since the last time we checked. All we need for that is a flag, not a queue. * refactor(watcher): skip is_interesting check if event already set * refactor(watcher): remove unused params from is_interesting Remove the unused `time` and `event_type` parameters from `Watcher.is_interesting()`. * refactor(watcher): delete lektor.watcher.watch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current implementation stores all unprocessed filesystem change events in an unbounded Queue. There can possibly be quite a number of events, so this can be (in some situations) a memory consumption issue.
(In addition, we were storing, event-type and path in the queue, though we never referenced it.)
All we really care about is whether there has been any interesting event since the last time we checked. All we need for that is a flag, not a queue. So here we replace the
queue.Queuewith athreading.Eventthreading.BoundedSemaphore.API Changes
This does change the API to our
Watcherclass. I think Lektor itself is the only user of that, however, so I don't think that's a big deal.We might as well remove the
lektor.watcher.watchfunction, too. It's just as easy to use theWatcherclass directly.Other Changes
There's logic in the current code that, for successful builds, only check for file changes that occur after the end of the last successful build. (For failed builds, this logic is bypassed, so any unprocessed changes will trigger a new build-all.)
I think both of these are not quite correct. Really we want to check for any changes since the beginning of the previous build — whether or not it was successful.
Issue(s) Resolved
Fixes #
Related Issues / Links
Depends on #1117
Description of Changes