apache / karaf-minho

Apache Karaf Minho
https://karaf.apache.org/
Apache License 2.0
6 stars 5 forks source link

[#33] Add Minho.getInstance() convenient method to access Minho running instance #34

Closed jbonofre closed 1 year ago

rmannibucau commented 1 year ago

Hi,

Wonder if this shouldn't be reverted since it open doors to a lot of issues like a leak (close hook does not reset the instance), concurrency inconsistency (you can get > 1 minho instance, which one is right?).

I propose to move it to a service (GlobalInstanceService) which would be usable with something like GlobalInstanceService.get().getMinho(). The service wouldn't be auto-registered but explicitly registered using Minho builder API.

Wdyt?

jbonofre commented 1 year ago

Some background about this change.

Imagine an user expose a REST endpoint using minho-rest (so Jersey JAXRS) and doesn't create the Minho instance (it's just use minho-boot main.

All jar in a folder, it just launches the runtime with java -jar minho-boot.jar.

So, the user needs a way to retrieve the Minho instance created in the minho-boot Main.

Imagine, in the JAXRS class:

@Path("/")
public class MyApi {

 @Path("/foo")
 @GET
 public String foo() {
   return Minho.getInstance().getServiceRegistry().get(Foo.class).foo();
 }
}

Thoughts ?

rmannibucau commented 1 year ago

@jbonofre think it can be implemented this way - explicit enablement: https://github.com/apache/karaf-minho/pull/36/files#diff-76162c6ce6856187daf0c3c93d0edf7627f5b14cbc6b50a24e346d115124072f That said I think it is never useful - at least we, as dev, should target it. If an user wants that it should also register a JAXRS contextresolver/contextprovider which resolves to minho. In the PR mentionned previously I add Minho as a service so any service can access it and make it accessible to its components. Sound safer to me, wdyt?

jbonofre commented 1 year ago

@rmannibucau yes, using an user service to retrieve Minho instance from the registry could work too.