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.

Sign up for GitHub

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

Jump to bottom

filewatch: avoid accepting stale/duplicate events #4397

Merged
merged 3 commits into from
Apr 2, 2021
Merged

filewatch: avoid accepting stale/duplicate events #4397

merged 3 commits into from
Apr 2, 2021

Commits on Apr 2, 2021

  1. filewatch: avoid accepting stale/duplicate events 

    This fixes the somewhat legitimate flakiness experienced by one of
the tests (`TestCrashRebuildTwoContainersTwoImages`).

Previously, we added logic to avoid missed live updates, but the
safer logic means that it's possible to get a duplicate change:
in the case of the test that was failing intermittently, it had two
image targets that watched overlapping sets of files. If `foo` is
changed, once `img1` target sees the event, it adds a record to its
pending file set for `time.Now()`, and a build is triggered.
Then, `img2` target sees the *same* event, and adds a record to its
pending file set for `time.Now()`, which will _intentionally_ not
be cleared after the build finishes because it appears to have been
a change that came from _after_ the build was started. As a result,
it'll end up triggering another build.

The revised logic in this commit tries to handle cases safely but
also avoid redundant triggered builds:
 1) The event pre-dates the start time of the last finished build, so
    just drop it. (In practice, this is exceedingly unlikely to happen
    because it would mean an event was very delayed.)
 2) There's a current build in progress
      * If it was started _after_ the file was seen AND already knows
        about the file, just ignore it. (This handles the case of the
        overlapping target watch sets as in the flaky test.)
      * Otherwise, use `time.Now()` so that it won't be erroneously
        cleared at the finish of the build. (This is the missed live
        update case.)

It's still possible to get a redundant build in some cases, but that
is impossible to avoid reliably given the current logic. Since we do
make an effort to batch file events (within a target), the most likely
cause would be two image targets with non-overlapping changed files,
where the later-to-be-processed file change happened before the build
was started, but we use `time.Now()` since it isn't in the list of
affected files for the build. The likelihood of multiple almost
instantaneous changes to files across targets is low enough that this
is still an acceptable trade-off vs the alternative of missing updates.

If extra builds become a common complaint, we could add a slight
waiting period of ~100ms or so after the latest file change before we
would start a build to increase the likelihood that changes across
targets have successfully propagated.
    @milas
    milas committed Apr 2, 2021
    Configuration menu
    Copy the full SHA
    b8bba46 View commit details
    Browse the repository at this point in the history

  2. tests: remove duplicative & unnecessary file change logic

    @milas
    milas committed Apr 2, 2021
    Configuration menu
    Copy the full SHA
    2e14e4e View commit details
    Browse the repository at this point in the history

  3. filewatch: don't drop even if older than last build 

    this might be breaking Windows tests?
    @milas
    milas committed Apr 2, 2021
    Configuration menu
    Copy the full SHA
    7110ecf View commit details
    Browse the repository at this point in the history