[Diagnostics] Refactor diagnostics track methods to handle background work automatically

[tonidero]

Description

Before this, callers of the diagnostics track methods had to handle concurrency and offloading the work to a background thread. This moves the work inside the DiagnosticsTracker, which makes the methods non-async anymore.

[tonidero]

I had to do this for this to work as expected and wait for the completion of the block. It's very hacky, but I think it's what we wanted? Would appreciate feedback to make sure this is ok!

[jamesrb1]

Before it would not wait for block() to run before returning, but with this change it does. Is that desired? If so, this can be done by calling await block() without anything else, no need for the semaphore/Task.

[tonidero]

I believe we can't do await block() or await Task because we're inside a non-async function right? We would need to make this the operation dispatcher async, but the whole point of this object is to dispatch tasks in non-async contexts I believe.

Lmk if you want to chat more about this! I could have missed something! Note that this semaphore is only for tests too.

[jamesrb1]

Oh sheesh, I saw the async on the block's signature and misread it as the function's. Semaphore is the way to go!

[tonidero]

This is the main change, now dispatching to a background thread happens internally.

[jamesrb1]

I don't think you need a diagnosticsDispatcher? You could just create a Task here,

func track(_ event: DiagnosticsEvent) {
    Task {
        await self.clearDiagnosticsFileIfTooBig()
        await self.diagnosticsFileHandler.appendEvent(diagnosticsEvent: event)
    }
}

However, with this change, events submitted to track() aren't guaranteed to run sequentially as they are with the OperationDispatcher. But this guarantee won't have existed before, so unless you want to make that change, I think using Task is the way to go.

[tonidero]

There are 2 main reasons I decided to go with the OperationDispatcher:

• First, as you said, this will make the operations run sequentially, which I believe would be safer, since there could be chances of changing contexts between tracking different events, and if we reach the file too big issue, we might be missing extra data. As you said, this problem would be present right now.
• Second, I wanted to be able to test this class. If I used Task, I don't have a good way to determine when the tracking finished to do the assertions. With OperationDispatcher, we can pass a MockOperationDispatcher in the tests that runs things synchronously (mostly), allowing us to do the assertions immediately.

Lmk if you would like to talk more about it!

[jamesrb1]

OK, sounds good!

[JayShortway]

Nice solution!