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

CloseableResource invocation order #2211

Closed
jflefebvre06 opened this issue Mar 11, 2020 · 6 comments · Fixed by #2387
Closed

CloseableResource invocation order #2211

jflefebvre06 opened this issue Mar 11, 2020 · 6 comments · Fixed by #2387

Comments

@jflefebvre06
Copy link

jflefebvre06 commented Mar 11, 2020

CloseableResources should be closed in the inverse order of insertion.

Actually close() is executed in random order, since the values are stored in a ConcurrentHashMap.

storedValues = new ConcurrentHashMap<>(4);

@sbrannen sbrannen changed the title Closable resources execution order CloseableResource invocation order Mar 11, 2020
@sbrannen sbrannen added this to the 5.7 M1 milestone Mar 11, 2020
@sbrannen
Copy link
Member

Tentatively slated for 5.7 M1 solely for the purpose of team discussion

@marcphilipp
Copy link
Member

@jflefebvre06 Could you please elaborate more on your concrete use case?

Would adding a single CloseableResource that closes your resources in the order you need be a viable workaround?

@bjhargrave
Copy link
Contributor

bjhargrave commented Aug 18, 2020

I have a use case from https://github.com/osgi/osgi-test.

We have an extension for bundle context, an extension for services which uses the bundle context extension, and another extension for configuration which uses the previous 2 extensions. So we use CloseableResources to clean up the resources consumed by each extension. But we end up with the CloseableResource for bundle context closed before the CloseableResource for services. It would be much cleaner and correct for the other order.

So ordering the CloseableResources in inverse insertion order would be most helpful for us.

An alternate would be to use the @Order annotation on the implementation of the CloseableResource.close() method to allow the implementor to influence the close order.

@bjhargrave
Copy link
Contributor

Would adding a single CloseableResource that closes your resources in the order you need be a viable workaround?

This is not possible for separate and cooperating extensions in the osgi-test scenario since each extension manages its own CloseableResource.

@kriegfrj
Copy link
Contributor

I have a use case from https://github.com/osgi/osgi-test.

We have an extension for bundle context, an extension for services which uses the bundle context extension, and another extension for configuration which uses the previous 2 extensions. So we use CloseableResources to clean up the resources consumed by each extension. But we end up with the CloseableResource for bundle context closed before the CloseableResource for services. It would be much cleaner and correct for the other order.

So ordering the CloseableResources in inverse insertion order would be most helpful for us.

An alternate would be to use the @Order annotation on the implementation of the CloseableResource.close() method to allow the implementor to influence the close order.

I think that guaranteeing cleanup in the reverse insertion order is the most sustainable solution here.

The problem with the @Order annotation approach is that it requires a "global" view of all the extensions/closeable resources involved and an understanding of their interdependencies so that you can set an order that makes sense for all of the components. This can work when the number of extensions is small and you have direct control over them (as is the case at the moment for osgi-test), but what happens if/when third parties start writing their own extensions that depend on ours? We could end up pushing responsibility for maintaining the correct @Order annotations onto the extension users.

I think of build systems - it has been a long time since build systems were specified as a linear list of build steps. For the most part now, they work by defining more-or-less-atomic build steps, each of which specifies its dependencies on other build steps (if any) - then letting the build system use this information to determine an appropriate build order. Bundle resolution in OSGi follows a similar pattern for the most part.

Cleaning up in reverse insertion order is a pretty neat way of solving any inter-dependency issues for most cases in a transparent way.

Another way I could see to do it would be to define a @DependsOn annotation that you can use to annotate extensions to explicitly declare their dependencies. We could even co-opt @ExtendsWith/@RegisterExtension for this purpose.

@bjhargrave
Copy link
Contributor

The problem with the @Order annotation approach is that it requires a "global" view of all the extensions/closeable resources involved and an understanding of their interdependencies so that you can set an order that makes sense for all of the components.

I don't think this is entirely true. You don't really need to establish a total ordering over all CloseableResources in an ExecutionContext's Store. You only need to establish some orderings between some interdependent CloseableResources.

For the OSGi crowd, this is like Start Levels. You don't need to define a total ordering of every bundle for the purposes of starting the bundles but you sometimes need to make sure bundle B start after Bundle A, so you need to establish a start level for Bundle B that is higher than that of Bundle A (which may just be the default start level).

In the specific use case I mention above, if, by default, all CloseableResources have an order of Order.DEFAULT, then to solve my ordering problem, I just need to set the order of the service extension's CloseableResource.close() method to @Order(Order.DEFAULT-1) to ensure it is invoked before the bundle context extension's CloseableResource.close() method. The ordering of other CloseableResources is not really important in this use case as they do not have any inter-dependencies.

But I think that inverse insertion order would be fine for my use case because I can ensure the bundle context extension's CloseableResource is inserted in the Store before the service extension's CloseableResource.

But I can imagine it is not always possible that one can control the insertion order to get the desired close order. (An extension might lazily insert a CloseableResource into the Store.) And so using Order annotations can provide control back to the developer of a given CloseableResource.

Another way I could see to do it would be to define a @DependsOn annotation that you can use to annotate extensions to explicitly declare their dependencies. We could even co-opt @ExtendsWith/@RegisterExtension for this purpose.

This is also an interesting idea and probably a better declarative way of expressing dependencies between extensions that could apply to much more then CloseableResource invocation order. (Although I am not sure, in the current implementation, the Store knows which extension is inserting a CloseableResource so that it can control their close invocation order. So more context would need to be captured for this level of knowledge.)

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

Successfully merging a pull request may close this issue.

5 participants