-
Notifications
You must be signed in to change notification settings - Fork 34
Improve Resource Cleanup #277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Merging with main has resulted in some test failures on my end. UPDATE: These seem like flaky tests, I see them failing roughly 1 out of every 3 runs |
facf91a to
f165385
Compare
|
Hi @wenxi-zeng, could you please take a look at this? |
wenxi-zeng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @apuig, thanks for the pr. this is great stuff. would you mind to address my feedback before I got it merged? I can do it on my end too, but just don't want to steal your idea.
| fun shutdown() { | ||
| fun shutdown(waitForTasks: Boolean = false) { | ||
| timeline.applyClosure { | ||
| if (it is com.segment.analytics.kotlin.core.platform.EventPipeline) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a shutdown method with empty body to the Plugin interface, so we can just call it.shutdown without knowing the existence of EventPipeline? then in SegmentDestination override the shutdown method to close EventPipeline
| store.shutdown(); | ||
| store.shutdown() | ||
|
|
||
| if (storage is StorageImpl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can also add a close method with empty body to Storage and in StorageImpl override it to close the eventStream. so we could just call storage.close without the check
This PR addresses critical problems caused by the use of JVM shutdownHooks in environments where analytics-kotlin is loaded via isolated class loaders (e.g., plugin systems). Relying on shutdownHooks in these scenarios leads to failures and class-loading errors during system shutdown, especially when plugins and their class loaders are retired before the JVM executes any registered hooks using classes not previously loaded.
No More Runtime.addShutdownHook
All references to shutdownHooks in EventStream and EventPipeline are gone. Cleanup and shutdown are now handled directly via the shutdown() call instead.
Plugin & Coroutine Cleanup:
All EventPipeline plugins get stopped before coroutines are shut down.
@JvmOverloadsso nothing breaks on the Java side.Potential issue
Now, shutdown needs to be called directly when you want cleanup. If for some reason this is not a good idea on Android, I recommend adding a shutdownHook in the android module.
Disclaimer 1
Didn’t get a chance to add a test for this, our integration tests seem to be working fine, but I’m not experienced with writing Kotlin tests or handling the analytics object lifecycle. I did give it a shot, just didn’t have much luck. If anyone can point me in the right direction, I’m happy to try again!