Skip to content

Conversation

@apuig
Copy link
Contributor

@apuig apuig commented Aug 2, 2025

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.

  • Once shutdown starts, no new analytics work gets accepted.
  • The global coroutine scope (analyticsScope) gets canceled, and shutdown can (optionally) wait for all analytics work to finish up.
    • The shutdown() method now has a waitForTasks parameter (defaults to false). If you set it to true, shutdown will block until all ongoing analytics tasks finish.
    • Java API compatibility is kept with @JvmOverloads so nothing breaks on the Java side.
  • Custom coroutine dispatchers are closed properly; The default Kotlin dispatchers will still stick around, though.

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!

@apuig
Copy link
Contributor Author

apuig commented Aug 4, 2025

Merging with main has resulted in some test failures on my end.

> Task :core:test

WaitingTests > test timeout force resume on DestinationPlugin() FAILED
    org.opentest4j.AssertionFailedError at WaitingTests.kt:162

WaitingTests > test timeout force resume() FAILED
    org.opentest4j.AssertionFailedError at WaitingTests.kt:97

346 tests completed, 2 failed, 10 skipped

UPDATE: These seem like flaky tests, I see them failing roughly 1 out of every 3 runs

@apuig apuig force-pushed the close-await-no-shutdownhook branch from facf91a to f165385 Compare December 11, 2025 16:04
@apuig
Copy link
Contributor Author

apuig commented Dec 11, 2025

Hi @wenxi-zeng, could you please take a look at this?
This issue is still affecting us and we are force to to maintain a list of 39 exotic Kotlin classes as a workaround, which is quite fragile and can easily break with any kotlin or kotlinx updates.
I’d appreciate your consideration on this.

Copy link
Contributor

@wenxi-zeng wenxi-zeng left a 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.

@apuig
Copy link
Contributor Author

apuig commented Dec 16, 2025

Thank you for the review @wenxi-zeng. It definitely looks cleaner now.

Copy link
Contributor

@wenxi-zeng wenxi-zeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! thanks for contributing and writing the unit tests as well

@wenxi-zeng wenxi-zeng merged commit c3499e6 into segmentio:main Dec 16, 2025
8 of 9 checks passed
@codecov
Copy link

codecov bot commented Dec 16, 2025

Codecov Report

❌ Patch coverage is 67.74194% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.03%. Comparing base (b958b36) to head (efd7f0d).
⚠️ Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
...ava/com/segment/analytics/kotlin/core/Analytics.kt 50.00% 2 Missing and 5 partials ⚠️
.../java/com/segment/analytics/kotlin/core/Storage.kt 0.00% 1 Missing ⚠️
...kotlin/core/platform/plugins/SegmentDestination.kt 50.00% 0 Missing and 1 partial ⚠️
...ent/analytics/kotlin/core/utilities/StorageImpl.kt 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #277      +/-   ##
============================================
- Coverage     78.13%   74.03%   -4.10%     
- Complexity      592      714     +122     
============================================
  Files            80       93      +13     
  Lines          7275     8862    +1587     
  Branches        922     1377     +455     
============================================
+ Hits           5684     6561     +877     
- Misses          861     1153     +292     
- Partials        730     1148     +418     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants