[Diagnostics] Refactor diagnostics track methods to handle background work automatically #4270
Conversation
| } | ||
|
|
||
| semaphore.wait() |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh sheesh, I saw the async on the block's signature and misread it as the function's. Semaphore is the way to go!
| await self.clearDiagnosticsFileIfTooBig() | ||
| await self.diagnosticsFileHandler.appendEvent(diagnosticsEvent: event) | ||
| func track(_ event: DiagnosticsEvent) { | ||
| self.diagnosticsDispatcher.dispatchOnWorkerThread { |
There was a problem hiding this comment.
This is the main change, now dispatching to a background thread happens internally.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
tonidero
force-pushed
the
diagnostics-add-apple-products-request-event
branch
from
29c87ec to
818c18a
Compare
tonidero
force-pushed
the
diagnostics-make-non-async
branch
from
7d3be25 to
920c44a
Compare
tonidero
force-pushed
the
diagnostics-add-apple-products-request-event
branch
from
818c18a to
c970d78
Compare
tonidero
force-pushed
the
diagnostics-make-non-async
branch
from
920c44a to
a12f291
Compare
tonidero
force-pushed
the
diagnostics-make-non-async
branch
from
46cc2de to
7217cb8
Compare
|
Going to merge this one now that the previous PRs have been merged. No significant changes have happened in this PR so we should be good |
… work automatically (#4270) * Refactor diagnostics track methods to handle background work automaticaly * fix swiftlint
… work automatically (RevenueCat#4270) * Refactor diagnostics track methods to handle background work automaticaly * fix swiftlint
Description
Before this, callers of the diagnostics
trackmethods had to handle concurrency and offloading the work to a background thread. This moves the work inside theDiagnosticsTracker, which makes the methods non-async anymore.