[core] Ensure onClose callbacks are invoked in reversed initialization order #2020
+230
−13
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
update KoinExtension.kt
shchuko
force-pushed
the
yaroshchuk/on-close-order
branch
3 times, most recently
from
6081a6c
to
9401779
Compare
shchuko
changed the title
shchuko
force-pushed
the
yaroshchuk/on-close-order
branch
from
9401779
to
7f8f830
Compare
`onClose` callback can be used to release allocated resources
like DB connections.
Some beans can depend on each other.
An example: classic Web application consists of
`Controller`, `Service`, `Database`.
In this case, instantiation order is:
1. `Database`
2. `Service` - implements business logic, requires `Database`
3. `Controller` - REST controller, requires `Service`
On application graceful shutdown, `onClose` invocation order
must be reversed:
1. `Controller` - created last, destroyed first
2. `Service` - destroyed after `Controller` to avoid use-after-close
3. `Database` - destroyed after `Service` to avoid use-after-close
This commit delivers implementation of `onClose` calls ordering.
When instance factory creates its first instance, it means that
all dependencies are also created. So it's enough to sort factories
by 'first instance creation,' and then reverse the sequence when
invoking `onClose`.
Implementation:
* global 'instance creation order' atomic counter is created
* each instance factory has 'instance creation order' position
* when factory creates its first instance
* 'instance creation order' counter is incremented
* the counter's value is saved into 'instance creation order'
position of the factory
* factories can be sorted by 'instance creation order' position,
this can be used to order `onClose` calls
shchuko
force-pushed
the
yaroshchuk/on-close-order
branch
from
7f8f830
to
7cfa056
Compare
| @@ -28,8 +30,12 @@ import org.koin.mp.Lockable | |||
| /** | |||
| * Koin Instance Holder | |||
| * create/get/release an instance of given definition | |||
| * | |||
| * Implements [Comparable] to provide factories soring by instance creation order | |||
|
|
||
| // it's required to have `Long.MAX_VALUE` instantiations happened to make this check fail, | ||
| // which is not likely to happen | ||
| check(instanceCreationOrderPosition < Long.MAX_VALUE) { |
There was a problem hiding this comment.
instanceCreationOrderPosition default value is MAX_VALUE and this check throw an exception if it hit MAX_VALUE.
I'm not sure if I understand your code comment above and it seems error prone.
| abstract class InstanceFactory<T>(val beanDefinition: BeanDefinition<T>) : Lockable() { | ||
| abstract class InstanceFactory<T>(val beanDefinition: BeanDefinition<T>) : Lockable(), Comparable<InstanceFactory<*>> { | ||
| private val instanceCreationOrderSet = AtomicBoolean(false) | ||
| private var instanceCreationOrderPosition = Long.MAX_VALUE |
There was a problem hiding this comment.
The current design ties a child instance natural comparison to its insertion ordering within a parent collection. A Comparable is documented as a way to define a class's natural order, not their insertion order. See Comparable and Natural Sort Order.
Instead, what if the InstanceRegistry would handle the instances collection and closing ordering?
Here's a possible approach:
- The
InstanceRegistrycould use aConcurrentLinkedDequeto store keys, ensuring thread-safe ordering of factory registrations. - The
InstanceRegistrywould then access the deque to order the factory instances and close them in the correct order.
There was a problem hiding this comment.
same thought here. Consider another proposal @shchuko
arnaudgiuliani
requested changes
Nov 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1510
onClosecallback can be used to release allocated resourceslike DB connections. Discussion #1507
Some beans can depend on each other.
An example: classic Web application consists of
Controller,Service,Database.In this case, instantiation order is:
1.
Database2.
Service- implements business logic, requiresDatabase3.
Controller- REST controller, requiresServiceOn application graceful shutdown,
onCloseinvocation order must be reversed:1.
Controller- created last, destroyed first2.
Service- destroyed afterControllerto avoid use-after-close3.
Database- destroyed afterServiceto avoid use-after-closeThis commit delivers implementation of
onClosecalls ordering.When instance factory creates its first instance, it means that all dependencies are also created. So it's enough to sort factories by 'first instance creation,' and then reverse the sequence when invoking
onClose.Implementation:
onClosecalls