-
-
Notifications
You must be signed in to change notification settings - Fork 19
Notice when Team Collection monitoring stops working, and go Disconnected (BL-16729) #8338
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
Open
StephenMcConnel
wants to merge
11
commits into
Version6.5
Choose a base branch
from
BL-16729-TeamCollectionWhenDropboxStops
base: Version6.5
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
1f32780
Notice when Team Collection monitoring stops working, and go Disconne…
StephenMcConnel 815a595
Merge remote-tracking branch 'origin/Version6.5' into BL-16729-TeamCo…
StephenMcConnel 41a8ba4
Address Devin review: watcher-callback thread safety, and test the he…
StephenMcConnel 7c34357
Address Devin round 2: probe-exception strike, stale overflow write, …
StephenMcConnel 8b99f7f
Address Devin round 3: disconnect race, message-log snapshots, droppe…
StephenMcConnel 2baccca
Address Devin round 4: a failed sync no longer disables the heartbeat…
StephenMcConnel 08fd8cb
Start watching the Books folder if Dropbox delivers it later (BL-16729)
StephenMcConnel c956dda
Persist log messages under the lock, so the file matches memory (BL-1…
StephenMcConnel a7cddff
Don't let a failed OpenForms lookup swallow a disconnect (BL-16729)
StephenMcConnel a814635
Marshal via the UI SynchronizationContext, not a form lookup (BL-16729)
StephenMcConnel 6790f07
Correct the comment on why the Books watcher can be deferred (BL-16729)
StephenMcConnel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
|
StephenMcConnel marked this conversation as resolved.
|
This file contains hidden or 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
|
StephenMcConnel marked this conversation as resolved.
|
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| namespace Bloom.TeamCollection | ||
| { | ||
| /// <summary> | ||
| /// Decides when a run of failed connection checks has gone on long enough to believe. | ||
| /// Kept as a separate, pure class (no IO, no timers, no threads) so the policy can be | ||
| /// unit tested exhaustively; ConnectionHeartbeat supplies the timing. See BL-16729. | ||
| /// | ||
| /// The point of waiting for a second failure is that the things CheckConnection looks at | ||
| /// can lie: one dropped packet fails the probe to dropbox.com, a Wi-Fi roam briefly makes | ||
| /// NetworkInterface.GetIsNetworkAvailable() false, a flaky SMB share can answer "no such | ||
| /// folder" for a moment. Wrongly disconnecting a collection that is working is the worst | ||
| /// outcome available to us, because there is no automatic way back: the user has to | ||
| /// Reload Collection. Waiting fifteen seconds to be sure is cheap by comparison. | ||
| /// </summary> | ||
| internal class ConnectionFailureTracker | ||
| { | ||
| /// <summary> | ||
| /// How many checks in a row must report the same problem before we act on it. | ||
| /// </summary> | ||
| internal const int kRequiredConsecutiveFailures = 2; | ||
|
|
||
| private string _lastFailureL10nId; | ||
| private int _consecutiveFailures; | ||
|
|
||
| /// <summary> | ||
| /// Feed in the result of one connection check. Returns true when we have now seen | ||
| /// enough consecutive failures of the same kind to conclude we really are disconnected. | ||
| /// </summary> | ||
| /// <param name="problemOrNull">What CheckConnection returned: null means all is well.</param> | ||
| public bool RecordResult(TeamCollectionMessage problemOrNull) | ||
| { | ||
| if (problemOrNull == null) | ||
| { | ||
| Reset(); | ||
| return false; | ||
| } | ||
| if (problemOrNull.L10NId != _lastFailureL10nId) | ||
| { | ||
| // A different problem from last time. "No network" followed by "repo missing" | ||
| // is two transients, not one sustained outage, so start counting again. | ||
| _lastFailureL10nId = problemOrNull.L10NId; | ||
| _consecutiveFailures = 0; | ||
| } | ||
| return ++_consecutiveFailures >= kRequiredConsecutiveFailures; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Forget any run of failures, e.g. because we stopped checking for a while. | ||
| /// </summary> | ||
| public void Reset() | ||
| { | ||
| _consecutiveFailures = 0; | ||
| _lastFailureL10nId = null; | ||
| } | ||
| } | ||
| } |
|
StephenMcConnel marked this conversation as resolved.
StephenMcConnel marked this conversation as resolved.
StephenMcConnel marked this conversation as resolved.
|
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,166 @@ | ||
| using System; | ||
| using System.Threading; | ||
|
|
||
| namespace Bloom.TeamCollection | ||
| { | ||
| /// <summary> | ||
| /// Periodically re-checks that we can still reach the Team Collection repo, and disconnects | ||
| /// if we can't. See BL-16729. | ||
| /// | ||
| /// The file system watchers tell us at once when the shared folder is yanked away, but they | ||
| /// cannot tell us that Dropbox has stopped syncing: the folder is still there, we simply | ||
| /// stop receiving other people's work. Before this, CheckConnection ran only when the user | ||
| /// did something (check out, check in, delete), so somebody who was just reading and editing | ||
| /// their own checked-out book could go the whole session without finding out. | ||
| /// | ||
| /// Owned by TeamCollection.StartMonitoring/StopMonitoring, which gets the lifecycle right | ||
| /// for free: no heartbeat during SyncAtStartup (monitoring is deliberately off then), none | ||
| /// on a DisconnectedTeamCollection (whose Start/StopMonitoring are no-ops), and it stops | ||
| /// when we disconnect or dispose. | ||
| /// </summary> | ||
| internal sealed class ConnectionHeartbeat : IDisposable | ||
| { | ||
| /// <summary> | ||
| /// How long between checks when everything is fine. Long enough that the cost (which for | ||
| /// a Dropbox repo includes an HTTP HEAD to dropbox.com) is negligible, short enough that | ||
| /// the user finds out reasonably soon that they have stopped seeing their teammates' work. | ||
| /// Not const, and internal, so tests can shorten it. | ||
| /// </summary> | ||
| internal static int IntervalMs = 60 * 1000; | ||
|
|
||
| /// <summary> | ||
| /// How soon we look again after a check fails, to see whether it was just a blip. | ||
| /// Must be longer than DropboxUtils' 10-second cache of the dropbox.com probe, or the | ||
| /// second look would just return the first one's answer and confirm nothing. | ||
| /// </summary> | ||
| internal const int kConfirmIntervalMs = 15 * 1000; | ||
|
|
||
| private readonly TeamCollection _teamCollection; | ||
| private readonly ConnectionFailureTracker _tracker = new ConnectionFailureTracker(); | ||
| private Timer _timer; | ||
| private volatile bool _disposed; | ||
|
|
||
| public ConnectionHeartbeat(TeamCollection teamCollection) | ||
| { | ||
| _teamCollection = teamCollection; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Begin checking. Does nothing under unit tests, which must not be left with live | ||
| /// threadpool timers checking real folders and the real network. | ||
| /// </summary> | ||
| internal void Start() | ||
| { | ||
| if (Program.RunningUnitTests) | ||
| return; | ||
| // A one-shot timer that re-arms itself at the end of each tick (rather than a | ||
| // repeating one) makes overlapping ticks structurally impossible, so a probe that | ||
| // blocks for forty seconds on a dead share cannot pile up behind itself. | ||
| _timer = new Timer(Tick, null, IntervalMs, Timeout.Infinite); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Runs on a threadpool thread. Internal so tests can drive the policy directly, with | ||
| /// no timer involved. | ||
| /// </summary> | ||
| internal void Tick(object unused) | ||
| { | ||
| var delayUntilNextTick = IntervalMs; | ||
| try | ||
| { | ||
| if (!OkToCheckNow()) | ||
| { | ||
| // Not the live collection, not watching just now (e.g. during a sync), or | ||
| // busy writing to the repo. Anything we noticed before such a gap is no | ||
| // longer part of a "consecutive" run, and an in-flight write reports its | ||
| // own failures, so disconnecting out from under it would be disruptive. | ||
| _tracker.Reset(); | ||
| } | ||
| else | ||
| { | ||
| // A joiner's Books folder can arrive minutes after Bloom starts, once | ||
| // Dropbox delivers it. If we could not watch it then, this is where we | ||
| // notice it has turned up. See BL-16729. | ||
| _teamCollection.RetryDeferredWatching(); | ||
|
|
||
| // Deliberately the quiet overload: History messages are not de-duplicated, | ||
| // so a probe that wrote them would fill log.txt and raise a status-changed | ||
| // event on every tick of a perfectly healthy session. | ||
| var problem = _teamCollection.CheckConnection(writeHistoryMessages: false); | ||
| // The probe can block for several seconds (DropboxUtils allows 5 for its | ||
| // request to dropbox.com), which is long enough for a check-in or a sync to | ||
| // have started meanwhile. Re-ask before acting on what we found, or we | ||
| // would disconnect in the middle of one. | ||
| if (!OkToCheckNow()) | ||
| { | ||
| _tracker.Reset(); | ||
| } | ||
| else if (_tracker.RecordResult(problem)) | ||
| { | ||
| // Logged as well as acted on: without this, someone testing a real | ||
| // outage has no way to tell "the check ran and decided" from "the | ||
| // check never ran". | ||
| SIL.Reporting.Logger.WriteEvent( | ||
| $"Team Collection periodic check confirmed a problem ({problem.L10NId}); disconnecting." | ||
| ); | ||
| _teamCollection.ReportConnectionProblem(problem); | ||
| } | ||
| else if (problem != null) | ||
| { | ||
| SIL.Reporting.Logger.WriteEvent( | ||
| $"Team Collection periodic check found a problem ({problem.L10NId}); " | ||
| + $"looking again in {kConfirmIntervalMs / 1000}s before believing it." | ||
| ); | ||
| delayUntilNextTick = kConfirmIntervalMs; // suspicious; confirm sooner | ||
| } | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| // An exception here is not evidence that the repo is gone, so we don't | ||
| // disconnect over it. This matches TeamCollectionManager.CheckConnection. | ||
| NonFatalProblem.ReportSentryOnly(ex); | ||
| // It is also not evidence that the repo is FINE, so this tick tells us nothing | ||
| // either way -- which means it breaks the run. Without this reset, a failure, | ||
| // then a throwing probe, then another failure would count as two consecutive | ||
| // failures and disconnect a collection that was never shown to be unreachable | ||
| // twice in a row. | ||
| _tracker.Reset(); | ||
| } | ||
| finally | ||
| { | ||
| if (!_disposed) | ||
| { | ||
| try | ||
| { | ||
| _timer?.Change(delayUntilNextTick, Timeout.Infinite); | ||
| } | ||
| catch (ObjectDisposedException) | ||
| { | ||
| // Disposed while we were checking. Nothing to re-arm. | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Whether this is a sensible moment to check the connection at all. Checked both | ||
| /// before and after the probe, because the probe can block long enough for the answer | ||
| /// to change. | ||
| /// </summary> | ||
| private bool OkToCheckNow() | ||
| { | ||
| return !_disposed | ||
| && _teamCollection.IsMonitoring | ||
| && _teamCollection.IsLiveCollection | ||
| && !_teamCollection.IsWritingToRepo; | ||
| } | ||
|
|
||
| public void Dispose() | ||
| { | ||
| _disposed = true; | ||
| _timer?.Dispose(); | ||
| _timer = null; | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.