sim1222/osukey
1
1
Fork 0
mirror of https://github.com/osukey/osukey.git synced 2025-07-01 08:20:00 +09:00
Code Issues Packages Projects Releases Wiki Activity

Fix API request potentially firing failed events after completion

Browse Source 
Specifically, `Cancel()` calls were not thread safe. Due to a series of
events, `ListPollingComponent` could call `Cancel` from a non-update
thread, leading to a race condition where both a `Success` and `Fail`
event can be fired.

This is intended to be the simplest fix possible, locking and guarding
specifically on the callbacks. Further work could be done in the future
to improve the flow surrounding `pendingFailure`, potentially reducing
redundant work and cleaning up the code, but that's not happening here.

Closes https://github.com/ppy/osu/issues/13632.
This commit is contained in:
Dean Herbert
2021-07-19 20:27:00 +09:00
parent 23ef666f34
commit 80c2b1449b
4 changed files with 75 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files

58
osu.Game/Online/API/APIRequest.cs
View File

@ -79,7 +79,13 @@ namespace osu.Game.Online.API
/// </summary>
public event APIFailureHandler Failure;
private bool cancelled;
private readonly object completionStateLock = new object();
/// <summary>
/// The state of this request, from an outside perspective.
/// This is used to ensure correct notification events are fired.
/// </summary>
private APIRequestCompletionState completionState;
private Action pendingFailure;
@ -116,12 +122,7 @@ namespace osu.Game.Online.API
PostProcess();
API.Schedule(delegate
{
if (cancelled) return;
TriggerSuccess();
});
API.Schedule(TriggerSuccess);
}
/// <summary>
@ -131,16 +132,29 @@ namespace osu.Game.Online.API
{
}
private bool succeeded;
internal virtual void TriggerSuccess()
{
succeeded = true;
lock (completionStateLock)
{
if (completionState != APIRequestCompletionState.Waiting)
return;
completionState = APIRequestCompletionState.Completed;
}
Success?.Invoke();
}
internal void TriggerFailure(Exception e)
{
lock (completionStateLock)
{
if (completionState != APIRequestCompletionState.Waiting)
return;
completionState = APIRequestCompletionState.Failed;
}
Failure?.Invoke(e);
}
@ -148,10 +162,14 @@ namespace osu.Game.Online.API
public void Fail(Exception e)
{
if (succeeded || cancelled)
return;
lock (completionStateLock)
{
// while it doesn't matter if code following this check is run more than once,
// this avoids unnecessarily performing work where we are already sure the user has been informed.
if (completionState != APIRequestCompletionState.Waiting)
return;
}
cancelled = true;
WebRequest?.Abort();
string responseString = WebRequest?.GetResponseString();
@ -181,7 +199,11 @@ namespace osu.Game.Online.API
/// <returns>Whether we are in a failed or cancelled state.</returns>
private bool checkAndScheduleFailure()
{
if (pendingFailure == null) return cancelled;
lock (completionStateLock)
{
if (pendingFailure == null)
return completionState == APIRequestCompletionState.Failed;
}
if (API == null)
pendingFailure();
@ -199,14 +221,6 @@ namespace osu.Game.Online.API
}
}
public class APIException : InvalidOperationException
{
public APIException(string messsage, Exception innerException)
: base(messsage, innerException)
{
}
}
public delegate void APIFailureHandler(Exception e);
public delegate void APISuccessHandler();