search

Netflix / governator

Governator is a library of extensions and utilities that enhance Google Guice to provide: classpath scanning and automatic binding, lifecycle management, configuration to field mapping, field validation and parallelized object warmup.
Apache License 2.0
825 stars 180 forks source link

Pre destroy no scope bug #393

Closed tcellucci closed 5 years ago

tcellucci commented 5 years ago

what is the problem: fix a long-standing memory leak caused by unscoped instances injected via Provider. The situation commonly occurs when a provider is declared for a type that implements AutoCloseable or is annotated with '@PreDestroy'. At runtime, the injected instances are bound with 'no scope' by Guice, leading to a large number of cleanup actions pointing to potentially the same object. The memory leak consists of the cleanup actions themselves, rather than the provided instance.

what is the fix: this PR adds specific handling of ProvidedInstanceBindings, to ensure that cleanup actions are not created every time a provider-sourced instance is injected. It also refactors the PreDestroyMonitor class to hold any cleanup actions for 'unscoped' instances in a WeakHashMap, to ensure that only one cleanup action exists for an unscoped provisioned instance.

The PR also introduces some code to log the number of unscoped instances present at shutdown, and shows where these instances were bound to the injector.

brharrington commented 5 years ago

It seems like this is trying to treat objects with no scope as having singleton scope unless they happen to get collected prior to shutdown. If the objects are collected, then they will not get shutdown properly which could result in some sort of resource leak. So generally speaking the user cannot rely on the lifecycle management of objects with no scope. Perhaps it would be less confusing and more consistent overall if Governator never manages the lifecycle of these objects with no scope rather than partially doing it. Is there a known use-case where this partial handling is preferable?

tcellucci commented 5 years ago

performing cleanup on surviving 'no scope' instances during injector shutdown is a long-standing feature and needs to remain for backward compatibility.
The reasoning at the time the feature was written is that Governator should make an effort to clean up any resources it creates, particularly since: (1) the default Guice provisioning is 'unscoped' and automatic binding is enabled (2) our home-built frameworks make heavy use of static internals that enable objects to outlive the injector (3) we call 'PostConstruct' on unscoped instances. It was deemed 'least astonishing' to also call PreDestroy.