Skip to content
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

Thread leak in ReadWriteFileConnection on recompile #1389

Open
PseudoKnight opened this issue Aug 3, 2024 · 1 comment
Open

Thread leak in ReadWriteFileConnection on recompile #1389

PseudoKnight opened this issue Aug 3, 2024 · 1 comment
Labels
bug Things that don't work as designed

Comments

@PseudoKnight
Copy link
Contributor

Under certain conditions, ReadWriteFileConnection threads will accumulate after recompile. Affects StringSerializableDataSources (YML, JSON, INI, CSV, XML) in PersistenceNetwork. On recompile, by default all DataSources in the DataSourceFactory cache are cleared. There's a secondary cache in ThreadsafeDataSource using a WeakHashMap. If it's cleared from there by the time of next get/store_value() call, a new DataSource is created. This causes the StringSerializableDataSource to create a new ReadWriteFileConnection ConnectionMixin, which initializes a new ThreadPoolExecutor with a single core thread. This ExecutorService won't shut down until there are no core threads and all references are lost. Thus we now have two (and counting) ReadWriteFileConnection threads.

Recreation steps:

  • Setup JSON (or other StringSerializableDataSource) connection in persistence.ini
  • get|store a value in this DataSource to create core thread in ReadWriteFileConnection
  • Recompile to clear data sources from DataSourceFactory static map
  • Force a gc to clear data sources from ThreadsafeDataSource static map
  • get|store same StringSerializableDataSource again

Solution depends on intended/desired behavior of these systems, but race conditions should probably be considered. Here's some ideas I explored before posting this:

  • Make ThreadPoolExecutor static in ReadWriteFileConnection (only ever one thread for these connection types)
  • Shutdown ThreadPoolExecutor in a new method that's called in StringSerializableDataSource.disconnect() and then await termination to ensure all tasks are complete
  • Set core threads to zero (simple, but doesn't help with race conditions)
  • Do not clear DataSources from DataSourceFactory (would keep around DataSources for removed connections, but this is rare; would need to make sure disconnect() doesn't break any DataSources)

This won't always affect users that use these connection types, but in the mean time users can workaround this leak by using /recompile -r to avoid reloading the Persistence Network.

@PseudoKnight PseudoKnight added the bug Things that don't work as designed label Aug 3, 2024
@PseudoKnight
Copy link
Contributor Author

PseudoKnight commented Aug 3, 2024

Lildirt's example of the thread accumulation they were experiencing:

https://paste.thezomg.com/212182/09916317/

This behavior wasn't immediately reproduceable on a clean installation. I suspect something was triggering gc in their ms scripts, and with multiple StringSerializableDataSources, this caused these waiting threads to accumulate quickly after recompiling many times ("100+").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Things that don't work as designed
Projects
None yet
Development

No branches or pull requests

1 participant