collect icon indicating copy to clipboard operation
collect copied to clipboard

Allow logging to audit log from background threads

Open seadowg opened this issue 2 years ago • 1 comments

Currently, using AuditEventLogger to log events from a background thread is dangerous as it could cause a ConcurrentModificationException due to the underlying ArrayList that events are stored in. After #5259, logging from a background thread will be protected by restricting logging to the UI thread (similar to how IO explodes when it runs on the UI thread in Android).

We might want to look at making our audit logging "thread safe" to prevent these kinds of problems. By that I mean:

  1. Events end up in the log in the order that AuditEventLogger#logEvent/flush is called
  2. Events added by AuditEventLogger#logEvent/flush always end up in the log (none are dropped/lost)

Additionally, we want to avoid the UI thread having to wait for another AuditEventLogger#logEvent/finalize call to complete.

As I'm writing this out, it does strike me that we may not actually want to ever call AuditEventLogger#logEvent/flush from a background thread: even if condition 1 is true, if we're calling from a background thread we will lose control of the order in which logEvent/finalize is called. We might simply be passing the concurrency problem up the call stack by doing this. If that's the case, it might be worth keeping the UI thread check we have and potentially just adding async writes so that we don't block the UI thread when logging/flushing like we do now.

Either way, this would be good to discuss!

seadowg avatar Aug 24 '22 12:08 seadowg

As part of this train of thought, I think we also want to think about moving logEvent/flush calls closer to the UI interactions they represent. For example, currently we log question events when rendering questions, rather than when the user switches between screens creating a pretty risky tie between rendering and the audit log. I might be forced to address this in my fix for #5240.

seadowg avatar Aug 24 '22 12:08 seadowg