Closed dkjepben closed 2 years ago
Hi @dkjepben, thank you for the bug report!
I tried to reproduce the behavior based on the steps that you outlined, but I wasn't successful in doing so. I used the demo app in this repository with AsyncEmitter. I tried closing the application by forcefully quitting it or by calling Application.Quit()
. However, opening the app immediately after didn't make it hang for me. I assume that there is some other influence that I am missing in my simple demo.
To help me understand the problem better, please bear with me with the following questions:
tracker.StopEventTracking()
when the application is quit, maybe in the OnApplicationQuit()
callback?Stop()
method in AsyncEmitter
?The AsyncEmitter
has a mechanism to gracefully stop the emitter and consumer threads. In the Stop()
function, it uses the emitLock
to notify the emitter thread to shut down and the payloadQueue
to notify the consumer thread. This is a similar mechanism as the cancellation token that you mentioned. I would prefer to avoid calling Abort on the threads and rather close them gracefully as that could cancel ongoing requests and increase the likelihood of an inconsistent state. It's preferable to finish sending events before app is quit.
Hi @matus-tomlein,
Thanks for the reply. We get it thread hang when we close the app by swipe-closing it on iOS (it only is reproducable on iOS devices). It could be a combination of our app logic and this that does the trick which cannot be reproduced in the demo app, then I know it can be difficult to fix.
To answer the points:
/// <summary>
/// Stops the Emitter and the Event Consumer.
/// </summary>
public override void Stop() {
if (sending) {
// Stop the emitter sending loop.
sending = false;
lock (emitLock) {
Monitor.Pulse(emitLock);
}
// Stops the event consumer.
consuming = false;
payloadQueue.Enqueue(null);
// Cancel threads
emitThread.Abort();
payloadConsumer.Abort();
}
}
Hope it helps and make sense, if not let me know.
Thanks for the information, @dkjepben! While I still couldn't reproduce the exact behavior, I could reproduce having the emitter thread not close properly when app is closed. There is a missing check for whether the emitter is still running in a loop in the emitter thread. If you are interested, you can see PR #47, it's just a one line change. Hopefully it will fix the problem that you are seeing.
Awesome, I will try to test if this fixes the issue in our app. Thank you very much, and will let you know the results.
I can confirm this fixed our issue (and in a better way than my own fix ;)), thank you very much.
Awesome, glad to hear that!
Describe the bug
When closing an app that uses the Snowplow tracker with an AsyncEmitter the threads are not properly closed in the AsyncEmitter class thus making the app hang for 5 seconds before the OS forcefully closes the app. This produces a crash report in xcode on our released app.
To Reproduce
Expected behavior
Its expected that the AsyncEmitter threads are closed when closing the app. This can be done implemented a cancellation token or calling Abort (properly best with a cancellation token).
Screenshots
No screenshots as the app just hangs.
Device and Build Information (please complete the following information):
Additional context
The method Stop() in AsyncEmitter line 103 is where I have made a local fix for the issue.