snowplow / snowplow-cpp-tracker

Snowplow event tracker for C++. Add analytics to your C++ applications, games and servers
http://snowplowanalytics.com
Apache License 2.0
9 stars 6 forks source link

The ClientSession destructor can take a very long time to execute #20

Closed OskarSigvardsson closed 2 years ago

OskarSigvardsson commented 3 years ago

The ClientSession background thread always unconditionally sleeps for check_interval milliseconds, which causes a big problem for the destructor: the destructor (because it has to join() the thread) has to wait for the ClientSession background thread to wake up before the background thread can exit. This can be quite serious: if you set check_interval to be 10 seconds, it means that the destructor can essentially lock up for 10 seconds. Since the destructor usually runs during shutdown, this can lock up shutdown of a program for quite a long time, which is a very poor experience for users.

The fix is to not use std::this_thread::sleep_for (which always unconditionally sleeps for a given amount of time), but to use a condition variable to sleep instead: the condition variable normally sleeps just like sleep_for, but if a certain condition is met (in this case this->m_is_running being false), it can be woken up early so that the thread can exit quickly.

I've already made a fix for this, but I saw in your contributing document that you maintain a strict 1-1 correspondence between issues and commits, so I figured I'd create an issue for it before creating the pull request.

OskarSigvardsson commented 3 years ago

Sorry for the message spam there ☝️ , I fixed a couple of issues with the commit (indentation, replacing tabs with spaces, etc.). I forced pushed into my own branch before creating the PR, i didn't realize that everytime i did that it would add stuff to this page :)

paulboocock commented 3 years ago

Thanks for reporting the issue. No worries about the commit spam, GitHub likes the keep us very informed!

matus-tomlein commented 2 years ago

This should no longer be a problem since we rearchitected how the ClientSession is updated, removing the daemon thread altogether. The change is released in version 0.2.0 of the tracker