Guard against work being queued on an already-disposed ThreadedTaskScheduler

[peppy]

Addresses https://github.com/ppy/osu/runs/7422392920?check_suite_focus=true#step:5:41

We already catch in all other cases.

Checking microsoft examples, generally TaskScheduler implementations don't even implement IDisposable, which means they will act similarly at process exit (dropping work). I think keeping the IDisposable implementation allows us better control and don't see it as an issue, so I'm not going to touch that. Logging should be enough IMO.

[smoogipoo]

What actually happens to an awaiter of this task? Is this effectively a memory leak since the awaiter will never return?

Something like:

Task.Run(async () =>
{
    await Task.Factory.StartNew(..., disposedTaskScheduler);
});

[peppy]

I guess cancelling the task will resolve this? Is that a solution you're okay with?

[peppy]

Actually I'm not sure if that's even possible...

That said, I'm not sure I'd classify it as a "memory leak". You'd hope anything waiting on a task would have a timeout. Open to alternative solutions either way.

[frenzibyte]

Upon researching on tasks and task schedulers, if the task has reached the TaskScheduler for queue, then it is expected for the task to eventually run, meaning that the TaskScheduler has no other way but to throw if it cannot run the task (see dotnet/runtime#24221).

And through testing around on the above scenario in a test case form, I've came to realise that if the task which is awaiting on the scheduled task is supplied with a cancellation token, then it'll bail out before attempting to await on the task (without having to explicitly use ThrowIfCancellationRequested).

In other words, we could potentially solve this by offering consumers a CancellationToken from TaskScheduler that's cancelled on scheduler disposal (diff). Or alternatively inform all consumers to ensure their tasks are cancelled before attempting to await on a scheduled task, via an exception message on QueueTask or otherwise (diff).

Interestingly enough, the former solution does not work when testing locally, and I have not the slightest clue on what's going on (it gets the point where if I pass an external cancellation token to the TaskScheduler and dispose it there, the test does not pass, but if I cancel outside before disposing scheduler, then it passes).

EDIT: Well, even the second solution fails intermittently...

[peppy]

Upon researching on tasks and task schedulers, if the task has reached the TaskScheduler for queue, then it is expected for the task to eventually run, meaning that the TaskScheduler has no other way but to throw if it cannot run the task (see dotnet/runtime#24221).

Unless the process is shutting down, where they won't be run. So it's pretty undefined.

Anyway this is our task schedule which we're using for our own purposes. I'd want the solution to be as simple as possible. If what I have proposed doesn't stick I'll just try-catch each and every usage or something like that.

[frenzibyte]

Would prefer try-catch to avoid the above mentioned scenario.

Also, upon taking a second look at the test failure, Task.Factory.StartNew(..., scheduler) will call scheduler.QueueTask inline:

• TaskFactory.StartNew -> Task.InternalStartNew -> Task.ScheduleAndStart -> TaskScheduler.InternalQueueTask -> TaskScheduler.QueueTask)

so if the scheduler is disposed at QueueTask, then BeatmapDifficultyCache must have been disposed already, i.e. I think this should've been handled at the cache class rather than at scheduler.

[peppy]

I think it's ugly as hell to try-catch an ObjectDisposed and I guarantee we'll forget to do it in other cases. But I guess I'm doing similar here for lack of better solution. Disposal is a stupid mechanic.

[smoogipoo]

It's very worrying that this can happen in the first place. Even worse is that we can't predict when this will happen by using an IsDisposed check, because the Disposal can happen on a completely different thread asynchronous to the QueueTask call.

Another option is to try-catch and queue onto the default task scheduler.

[peppy]

so if the scheduler is disposed at QueueTask, then BeatmapDifficultyCache must have been disposed already, i.e. I think this should've been handled at the cache class rather than at scheduler.

Well that is true, but it's also not easy to handle. Unless you're happy with scheduling all the calls in this and other cases (quite ugly IMO).

[frenzibyte]

Another option is to try-catch and queue onto the default task scheduler.

I had thought about doing that from inside QueueTask, but it doesn't seem possible as you can't call TaskScheduler.Default.QueueTask(task), and using Task.Start(TaskScheduler.Default) doesn't work either since the task is already in a started state, waiting to be queued to the scheduler.

[peppy]

I've updated this to ensure that tasks which are queued are eventually run (even if that run is synchronous).

[smoogipoo]

Let's hope we don't run into any deadlocks doing it this way.