diff --git a/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishSteps.tsx b/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishSteps.tsx index 67a543e4f364..924e88986b15 100644 --- a/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishSteps.tsx +++ b/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishSteps.tsx @@ -248,6 +248,8 @@ export const LibraryPublishSteps: React.FunctionComponent<{ function uploadOneBook() { setIsUploadComplete(false); setIsUploading(true); + cancelRequestedRef.current = false; + setHasAttemptBegun(false); // If either pre-upload request dies at the transport level we get no reply and no progress // message, so nothing else would ever clear isUploading. That used to leave only a stale // Cancel button, but now it would also keep the other publish tools disabled, so clear it @@ -260,6 +262,10 @@ export const LibraryPublishSteps: React.FunctionComponent<{ // The API already sent an error message return; } + // C# has now marked the attempt as started, so a Cancel from here on cannot be + // wiped out by that same request arriving late. Only now do we let the user + // press Cancel. See the note on hasAttemptBegun. + setHasAttemptBegun(true); get( "libraryPublish/getUploadCollisionInfo?index=" + conflictIndex, @@ -268,6 +274,12 @@ export const LibraryPublishSteps: React.FunctionComponent<{ // The API already sent an error message return; } + // The user cancelled while we were asking the server about an existing + // copy. That reply is no longer wanted: showing the collision dialog now + // would pop a dialog up over an attempt the user has abandoned, and + // posting the upload would only be declined. Either way we say nothing + // more -- C# has already reported the cancellation. (BL-16340) + if (cancelRequestedRef.current) return; if (result.data.shouldShow) { setUploadCollisionInfo(result.data); showUploadCollisionDialog(); @@ -294,9 +306,34 @@ export const LibraryPublishSteps: React.FunctionComponent<{ ); }; + // True from the moment the user clicks Cancel until C# reports what became of the upload. + // While it is true, UPLOAD BOOK is greyed out, so every path that ends an upload has to + // clear it. Missing one leaves the user with no working button at all and no way back + // short of leaving the screen, which is how BL-16340 presented. The two things that clear + // it are the uploadCanceled event and uploadSuccessful, and C# now guarantees one of them + // on every ending of a cancelled upload. + // + // Deliberately NOT cleared here on an error line: this fires for ANY Error-kind progress + // message, not just a terminal one, so clearing it here would re-enable UPLOAD BOOK while + // a cancel-pending upload was still running -- and pressing it would then clear the pending + // cancel server-side and start a second upload alongside the first. const [isCanceling, setIsCanceling] = useState(false); + + // True once C# has acknowledged the start of this attempt (the subscription check has come + // back). Until then Cancel is shown but disabled, because a cancel sent before C# knows an + // attempt exists races with the request that begins it -- and if the cancel loses that race + // it is wiped out and the book uploads anyway. Waiting for the acknowledgement removes the + // race by construction rather than narrowing it. (BL-16340) + const [hasAttemptBegun, setHasAttemptBegun] = useState(false); + + // Whether the user has cancelled the attempt that is currently starting up. This is a ref, + // not state, because the pre-upload callbacks below close over their render's values and + // would not see a state update made after they were created. + const cancelRequestedRef = useRef(false); + const handleUploadError = React.useCallback(() => { setIsUploading(false); + setHasAttemptBegun(false); }, []); useSubscribeToWebSocketForEvent( @@ -304,6 +341,7 @@ export const LibraryPublishSteps: React.FunctionComponent<{ kWebSocketEventId_uploadCanceled, () => { setIsCanceling(false); + setHasAttemptBegun(false); }, ); @@ -339,6 +377,11 @@ export const LibraryPublishSteps: React.FunctionComponent<{ kWebSocketEventId_uploadSuccessful, (results) => { setIsUploading(false); + // The user may have asked to cancel and C# finished anyway -- either the cancel + // arrived after the upload was committed, or it was delayed getting to C# at all. + // Whatever the reason, the upload is over, so the Cancel state has to end with it. + setIsCanceling(false); + setHasAttemptBegun(false); setBookUrl(results.url); setIsUploadComplete(true); }, @@ -629,9 +672,10 @@ export const LibraryPublishSteps: React.FunctionComponent<{ )} {isUploading ? ( { + cancelRequestedRef.current = true; setIsCanceling(true); setIsUploading(false); post("libraryPublish/cancel"); @@ -747,6 +791,7 @@ export const LibraryPublishSteps: React.FunctionComponent<{ {...uploadCollisionInfo} onCancel={() => { setIsUploading(false); + setHasAttemptBegun(false); }} conflictIndex={conflictIndex} setConflictIndex={changeConflictIndex} diff --git a/src/BloomExe/web/WebProgressAdapter.cs b/src/BloomExe/web/WebProgressAdapter.cs index 9ac3212f1fea..9ffedd4fc128 100644 --- a/src/BloomExe/web/WebProgressAdapter.cs +++ b/src/BloomExe/web/WebProgressAdapter.cs @@ -45,7 +45,17 @@ public bool ShowVerbose set { _showVerbose = value; } } - public bool CancelRequested { get; set; } + // Volatile because a cancel is set by an api handler on a server worker thread while the + // work being cancelled polls this from a background thread, with no lock between the two + // (BL-16340). An auto-property gives no such guarantee. + private volatile bool _cancelRequested; + + public bool CancelRequested + { + get { return _cancelRequested; } + set { _cancelRequested = value; } + } + public bool ErrorEncountered { get { return false; } diff --git a/src/BloomExe/web/controllers/LibraryPublishApi.cs b/src/BloomExe/web/controllers/LibraryPublishApi.cs index 121834ab5228..729cc1e2936a 100644 --- a/src/BloomExe/web/controllers/LibraryPublishApi.cs +++ b/src/BloomExe/web/controllers/LibraryPublishApi.cs @@ -103,7 +103,15 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) ); apiHandler.RegisterEndpointHandler("libraryPublish/setSummary", HandleSetSummary, true); apiHandler.RegisterEndpointHandler("libraryPublish/useSandbox", HandleUseSandbox, true); - apiHandler.RegisterEndpointHandler("libraryPublish/cancel", HandleCancel, true); + // Deliberately handled off the UI thread and without the api sync lock. All this + // handler does is set a flag, but its whole purpose is to interrupt an upload which + // may be monopolizing both the UI thread (offscreen-browser thumbnailing) and the + // lock for minutes at a time. If the cancel had to queue behind the very work it is + // trying to stop, the flag could get set only after the upload had passed its last + // cancellation check: the upload would run to completion and the user would be left + // with a Cancel that did nothing (BL-16340). Compare progress/cancel and + // signLanguage/cancelImportVideo, which are registered this way for the same reason. + apiHandler.RegisterEndpointHandler("libraryPublish/cancel", HandleCancel, false, false); apiHandler.RegisterEndpointHandler( "libraryPublish/getUploadCollisionInfo", HandleGetUploadCollisionInfo, @@ -163,12 +171,66 @@ private void HandleUseSandbox(ApiRequest request) request.ReplyWithBoolean(BookUpload.UseSandbox); } + /// + /// Ask the upload to stop. All we do is set the flag that the upload polls between its + /// stages; it can take a while to notice, since we don't interrupt a stage (notably PDF + /// creation) that is already under way. + /// private void HandleCancel(ApiRequest request) { - _progress.CancelRequested = true; + // Whether we announce the cancellation ourselves depends on how far the attempt has + // got. Only in StartingUp is there nobody else to do it: the client shows Cancel from + // the moment the user commits, but two API round trips (the subscription check and + // the "existing copy on server" query) happen before libraryPublish/upload arrives, + // so during those there is no upload running to notice the flag -- and the screen + // only ever leaves its Cancel state on such a report. While Uploading, UploadBookAsync + // reports whatever becomes of it. When Idle we must stay SILENT: the attempt is + // already over, and announcing a cancellation then would contradict the outcome the + // user was just given -- telling someone their upload was cancelled seconds after + // being told it succeeded is worse than saying nothing. (BL-16340) + UploadAttemptState stateWhenCancelled; + lock (_uploadStateLock) + { + _progress.CancelRequested = true; + stateWhenCancelled = _attemptState; + if (stateWhenCancelled == UploadAttemptState.StartingUp) + _attemptState = UploadAttemptState.Idle; // we are about to report it + } + + // Outside the lock: these send websocket messages, and the lock exists only to make + // the state read/write above indivisible. + if (stateWhenCancelled == UploadAttemptState.StartingUp) + { + ReportUploadCanceled(); + } + else if (stateWhenCancelled == UploadAttemptState.Idle) + { + // Nothing was running, so we have nothing to say about how it went -- but the + // screen may still be sitting in its Cancel state (an upload can finish in the + // moment between the user's click and this request arriving). Send the event + // alone: it releases the screen without claiming an upload was cancelled when + // it may in fact have just succeeded. + ReportUploadCanceled(withMessage: false); + } + // Uploading: UploadBookAsync reports whatever becomes of it. + request.PostSucceeded(); } + /// + /// Tell the user, and the publish screen, that the upload was cancelled. Sending the + /// event matters as much as showing the message: the screen greys out UPLOAD BOOK as + /// soon as the user clicks Cancel, and this event is the only thing that brings it back. + /// Pass false for withMessage when a message has already been given, to avoid a second + /// one -- the event still has to go out. + /// + private void ReportUploadCanceled(bool withMessage = true) + { + if (withMessage) + _webSocketProgress.Message("Cancelled", "Upload was cancelled", ProgressKind.Error); + _webSocketServer.SendEvent(kWebSocketContext, kWebSocketEventId_uploadCanceled); + } + private async Task HandleUpload(ApiRequest request) { await HandleUpload(request, false); @@ -181,26 +243,103 @@ private async Task HandleUploadWithNewUploader(ApiRequest request) private bool _changeUploader = false; + /// + /// How far the current upload attempt has got, as C# sees it. It exists so HandleCancel + /// can tell whether anyone else is going to report a cancellation -- and, just as + /// importantly, whether the attempt is already over and it should say nothing at all. + /// + private enum UploadAttemptState + { + /// No attempt is under way, or the last one's outcome has already been reported. + Idle, + + /// The client has committed to an upload and is making the pre-upload round trips, + /// but libraryPublish/upload has not arrived yet. + StartingUp, + + /// An upload is running; UploadBookAsync will report whatever becomes of it. + Uploading, + } + + private UploadAttemptState _attemptState = UploadAttemptState.Idle; + + // Guards _attemptState together with _progress.CancelRequested, so that reading one and + // writing the other is a single indivisible step. Without it the two could interleave + // such that HandleCancel reported the cancel AND the upload started anyway, so the + // cancel got reported twice. Only ever held across those field reads/writes -- never + // across the upload, the HTTP response, or a websocket send. + private readonly object _uploadStateLock = new object(); + private async Task HandleUpload(ApiRequest request, bool changeUploader) { if (request.HttpMethod == HttpMethods.Get) return; _changeUploader = changeUploader; - _progress.CancelRequested = false; - + // Note that we deliberately do NOT clear CancelRequested here. The user can click + // Cancel before this request arrives (see HandleCancel), and clearing it here would + // throw that cancel away and upload the book anyway (BL-16340). It is cleared in + // HandleCheckSubscriptionMatch, which the client calls at the start of every attempt. + bool alreadyCancelled; + lock (_uploadStateLock) + { + alreadyCancelled = _progress.CancelRequested; + if (!alreadyCancelled) + _attemptState = UploadAttemptState.Uploading; + } + if (alreadyCancelled) + { + // The user cancelled and this upload request is what is left of the attempt -- + // most often one the client had already posted, which then queued behind the UI + // thread while the lock-free cancel overtook it. HandleCancel has therefore + // already said "Upload was cancelled"; saying it again would put two identical + // red lines in the log for a single click. Send the event only: it costs nothing + // (the client just clears the same state again) and guarantees the screen is + // released even if this request is the last thing to happen. (BL-16340) + ReportUploadCanceled(withMessage: false); + request.PostSucceeded(); + return; + } + var toldClientTheOutcome = false; try { - await UploadBookAsync(); + toldClientTheOutcome = await UploadBookAsync(); } catch (Exception) { ReportTryAgainDuringUpload(); } + finally + { + // The attempt is over, so a cancel arriving from here on is too late to mean + // anything -- see HandleCancel. + bool mustStillReleaseTheScreen; + lock (_uploadStateLock) + { + _attemptState = UploadAttemptState.Idle; + + // A cancel can land after UploadBookAsync has already decided how this + // ended. If the upload then finished in a way that emits only an error line + // -- a failure or an exception -- nothing would ever release the screen from + // its Cancel state, and UPLOAD BOOK would stay greyed out for good: the + // original BL-16340 symptom, just through a narrower window. So make the end + // of every attempt reconcile. Sending the event twice is harmless (the + // client just clears the same state again); not sending it is not. + mustStillReleaseTheScreen = _progress.CancelRequested && !toldClientTheOutcome; + } + if (mustStillReleaseTheScreen) + ReportUploadCanceled(withMessage: false); // the failure was already reported + } request.PostSucceeded(); } - private async Task UploadBookAsync() + /// + /// Runs the upload and tells the user how it went. + /// + /// True if we sent the client an outcome EVENT (uploadSuccessful or + /// uploadCanceled) rather than only a progress message. The caller needs to know, + /// because the screen leaves its Cancel state only on such an event. + private async Task UploadBookAsync() { _webSocketProgress.Message("Common.Starting", "Starting..."); SetParentControlsState(false); // Disable UI @@ -251,28 +390,41 @@ private async Task UploadBookAsync() SetParentControlsState(true); // Re-enable UI } - if (_progress.CancelRequested) + // A cancel that arrived too late to stop anything must not be reported as though it + // had worked: if we got a book id back, the book really is on BloomLibrary now, and + // telling the user it was cancelled is a falsehood they might act on. uploadResult is + // empty when the upload was abandoned (including by the cancellation checks inside + // BookUpload) or failed, and "quiet" when a message has already been given. + var uploadActuallyCompleted = + !string.IsNullOrEmpty(uploadResult) && uploadResult != "quiet"; + if (_progress.CancelRequested && !uploadActuallyCompleted) { - _webSocketProgress.Message("Cancelled", "Upload was cancelled", ProgressKind.Error); - _webSocketServer.SendEvent(kWebSocketContext, kWebSocketEventId_uploadCanceled); - return; + // Send the event on EVERY cancelled ending, including the "quiet" one. The + // screen's Cancel state is cleared only by this event, and making it depend + // instead on some other path happening to emit an error line is how it gets + // left permanently greyed out -- the whole of BL-16340. For "quiet" a message + // has already been given, so send the event without adding a second one. + ReportUploadCanceled(withMessage: uploadResult != "quiet"); + return true; } if (caughtException != null) { ReportBasicErrorDuringUpload(); _webSocketProgress.Exception(caughtException); - return; + return false; } if (uploadResult == "quiet") { // no more reporting, sufficient message already given. + return false; } else if (string.IsNullOrEmpty(uploadResult)) { // Something went wrong, possibly already reported. ReportTryAgainDuringUpload(); + return false; } else { @@ -289,6 +441,7 @@ private async Task UploadBookAsync() kWebSocketEventId_uploadSuccessful, result ); + return true; } } @@ -340,6 +493,12 @@ private void HandleUploadCollection(ApiRequest request) return; } + // Bulk upload shares _progress with single-book upload but has no Cancel of its own, + // so a cancellation left over from an earlier single-book upload would silently stop + // it before it started. Taken under the same lock as every other write to this flag. + lock (_uploadStateLock) + _progress.CancelRequested = false; + Model.BulkUpload(Model.Book.CollectionSettings.FolderPath, _progress); request.PostSucceeded(); } @@ -352,6 +511,10 @@ private void HandleUploadFolderOfCollections(ApiRequest request) return; } + // See the note in HandleUploadCollection about why this is cleared here. + lock (_uploadStateLock) + _progress.CancelRequested = false; + var folderPath = request.RequiredPostString(); if (!string.IsNullOrEmpty(folderPath) && Directory.Exists(folderPath)) Model.BulkUpload(folderPath, _progress); @@ -415,6 +578,16 @@ private void HandleGetUploadCollisionInfo(ApiRequest request) private void HandleCheckSubscriptionMatch(ApiRequest request) { + // This is the first thing the client asks for once the user commits to an upload, so + // as far as C# is concerned it is where a new attempt begins -- and therefore where + // any cancellation left over from an earlier attempt is cleared. Deliberately not in + // HandleUpload, which arrives too late to clear it safely; see the note there. + lock (_uploadStateLock) + { + _progress.CancelRequested = false; + _attemptState = UploadAttemptState.StartingUp; + } + var subscriptionMatch = Model.CheckSubscriptionMatchBeforeUpload(); if (subscriptionMatch != null) { @@ -436,6 +609,19 @@ private void HandleCheckSubscriptionMatch(ApiRequest request) private async Task HandleUploadAfterChangingBookId(ApiRequest request) { + // Check for a pending cancel BEFORE changing anything. This is reached from the + // "already on BloomLibrary" dialog, which can still be on screen after the user + // cancelled; without this check we would give the book a brand-new instance id on + // disk and then decline to upload it, leaving its identity permanently changed for + // an upload that never happened. Event only, for the same reason as in HandleUpload: + // the cancellation has already been announced. (BL-16340) + if (_progress.CancelRequested) + { + ReportUploadCanceled(withMessage: false); + request.PostSucceeded(); + return; + } + if (!Model.ChangeBookInstanceId(_progress)) { request.Failed("Can't fix ID because in TC"); diff --git a/src/BloomTests/web/controllers/EndpointHandlerTests.cs b/src/BloomTests/web/controllers/EndpointHandlerTests.cs index 071121e6fe60..fe04116af557 100644 --- a/src/BloomTests/web/controllers/EndpointHandlerTests.cs +++ b/src/BloomTests/web/controllers/EndpointHandlerTests.cs @@ -1,4 +1,5 @@ using System.Threading; +using System.Threading.Tasks; using Bloom.Api; using Bloom.Book; using NUnit.Framework; @@ -85,6 +86,82 @@ public void Get_EndPointCaseIsIgnored() Assert.That(result, Is.EqualTo("OK")); } + /// + /// An endpoint whose job is to interrupt long-running work must be registered with + /// requiresSync false, or it queues behind the very request it exists to interrupt and + /// the flag it sets arrives too late to stop anything. libraryPublish/cancel is the case + /// this was written for (BL-16340); progress/cancel relies on the same property. + /// + /// Parking one request inside the lock needs a second worker to be free to serve the + /// interrupting one. BloomServer starts Math.Max(ProcessorCount, 2) of them, so there is + /// always at least one spare; were that ever reduced to a single worker this would fail + /// for a reason that has nothing to do with the lock. + /// + [Test] + public void RequestNotRequiringSync_IsServedWhileAnotherRequestHoldsTheSyncLock() + { + using (var slowRequestStarted = new ManualResetEventSlim()) + using (var letSlowRequestFinish = new ManualResetEventSlim()) + { + var slowRequestWasReleased = false; + _server.ApiHandler.RegisterEndpointHandler( + "test/slow", + request => + { + slowRequestStarted.Set(); + slowRequestWasReleased = letSlowRequestFinish.Wait(20000); + request.PostSucceeded(); + }, + handleOnUiThread: false, + requiresSync: true + ); + _server.ApiHandler.RegisterEndpointHandler( + "test/interrupt", + request => request.PostSucceeded(), + handleOnUiThread: false, + requiresSync: false + ); + + var slowRequest = Task.Run(() => + ApiTest.GetString(_server, "test/slow", timeoutInMilliseconds: 30000) + ); + // Sanity check: unless the slow request is actually running, it isn't holding the + // lock and the rest of the test would prove nothing. + Assert.That( + slowRequestStarted.Wait(10000), + Is.True, + "the slow request never started, so it never held the lock" + ); + Assert.That( + slowRequest.IsCompleted, + Is.False, + "the slow request should still have been in flight" + ); + + var interruptResult = ApiTest.GetString( + _server, + "test/interrupt", + timeoutInMilliseconds: 5000 + ); + + Assert.That(interruptResult, Is.EqualTo("OK")); + Assert.That( + slowRequest.IsCompleted, + Is.False, + "the interrupting request should have been served while the slow one still held the lock" + ); + + letSlowRequestFinish.Set(); + Assert.That( + slowRequest.Wait(20000), + Is.True, + "the slow request never finished once released" + ); + Assert.That(slowRequestWasReleased, Is.True); + Assert.That(slowRequest.Result, Is.EqualTo("OK")); + } + } + [Test] public void Get_Unrecognized_Throws() {