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

Not totally obvious how to do your own request scoping #85

Closed
mikehearn opened this issue Jan 5, 2021 · 14 comments
Closed

Not totally obvious how to do your own request scoping #85

mikehearn opened this issue Jan 5, 2021 · 14 comments
Assignees
Milestone

Comments

@mikehearn
Copy link

The docs talk about using factories/providers to do things like read environment variables, i.e. global statics. It's much less clear (because Avaje does it in an "auto magic" way) how you're meant to take objects from outside the injected world and insert them into a newly constructed graph of objects, or customize the "root" object with non-injected objects.

I thought the obvious approach would be to use the withBeans() method on the BeanContextBuilder but:

  1. The docs are quite clear this is only meant for testing.
  2. The generated code doesn't actually do this when receiving a new request. Instead it's using some sort of static create method that isn't mentioned anywhere. Presumably this is to avoid re-creating all singleton objects inside the request scope instead of just the controller.

As an IoC newbie it'd be nice if scoping was discussed a bit more in the docs. The current docs seem to assume everything is app scoped except this special case for specific frameworks and specific objects. That's no help if I want to use the library in a slightly different way.

@rbygrave
Copy link
Contributor

rbygrave commented Jan 5, 2021

Hi Mike, I am on holiday for another couple of weeks. As a quick response your observations are on the money regards request scope.

avoid re-creating all singleton objects inside the request scope

Exactly and as observed this implementation is detected and supported with generated code/factories/a bit magical (and ideally we could desire this to be more pluggable).

Feel free to add more comments but I won't be too responsive for 2 more weeks.

Cheers, Rob.

@mikehearn
Copy link
Author

That's no problem. I'm just exploring. You have a great project here - the zero open bug count is highly impressive, as is the 100% JavaDoc coverage. In the past I worked on Gmail where we were badly burned by Guice (in my view at least), and this has put me off IoC/DI frameworks for a long time. Given their enduring popularity I figured I should try and find one that was easily understandable and I liked. Avaje Inject is the closest I found so far, given that it's usable standalone and the feature set doesn't require a book to explain :)

Intuitively, I'd expect this to work by allowing a BeanContext to simply use beans from another BeanContext in preference to constructing one. It seems this already exists via the module system, sort of. Incidentally, I found the docs for this a bit opaque as well - the docs assert that beans have to be constructed taking modules into account but leaves it to the reader to figure out why. If we think of a "module" as a classical Java JAR where it's really just a bag of classes, why is that necessary - isn't the point of dependency injection to figure out dependency orderings for you already? Is this a side effect of the ahead-of-time compilation model, meaning there's no central point that figures out a unified cross-module construction "plan", leaving the developer to do it instead? If yes, I wonder if the JPMS could be utilised to figure this out automatically for a modularised app, as it seems like currently Avaje Inject defines its own mini-JPMS with its own notion of dependencies. The duplication seems like a pity.

@rbygrave
Copy link
Contributor

Ok. I'm thinking there are a few different parts to this so lets see how we go.

if I want to use the library in a slightly different way.

It might be good to get some background on what you are looking to do. I'm thinking more specifically on your needs/thinking around request scope (is this http requests or something different?).

The current docs seem to assume everything is app scoped except this special case for specific frameworks and specific objects.

With Avaje Inject it is really about Singleton and Request scope and "request scope" is orientated to how it works with JAX-RS (http) requests with the desire to mix injection of various http request/response objects with singleton scoped objects.

For me I'd say "request" scope can be completely avoiding by simply passing http request/response objects to a controller method (so the controller stays as a singleton and the request/response instances are always passed in as method arguments rather than being injected. In this sense and for me personally 99% of my JAXRS experience says we can have the controllers as singletons and just pass the request/response as needed so actual use of request scope for JAXRS can be very low indeed.

In this sense Avaje inject's "request scope support" is nice but there will be people who will literally never use it because they are happier keeping all their controllers as singleton scope and simply have the http request scope things like "Javalin Context" passed to the controller method.

current docs seem to assume everything is app scoped except this special case for specific frameworks and specific objects.

Yes. The support of the specific http frameworks is along the lines of "just works" by automatically determining that something injected mandates "request scope" and then generates factory methods to handle that so in this way there is nothing special or different developers need to do in terms of annotations etc for it to "just work" (but yeah then they need to review the generated source to see how it works).

@rbygrave
Copy link
Contributor

Re modules - Is this a side effect of the ahead-of-time compilation model, meaning there's no central point that figures out a unified cross-module construction "plan", leaving the developer to do it instead?

Yes exactly. So to say that again, per Jar is per compilation unit. The ordering of dependency injection is effectively know per jar / per compilation unit with avaje inject - some of these dependencies can be missing and provided by another module (or priority overridden by use of @Primary and @Secondary).

Comparing to say Spring where the ordering is all determined at runtime will ALL dependencies across the whole class path.

With avaje inject we need to control the order in which the modules are wired manually/explicitly (via @ContextModule). I don't see anything in JPMS that changes that part (the order the modules should be wired).

@mikehearn
Copy link
Author

My thinking was that JPMS gives the dependency graph of each module, so wiring could proceed bottoms-up by just re-using the module graph rather than requiring the user to re-specify it. I think you're saying there's some subtlety that makes that infeasible though?

The idea I was exploring was an approach to server construction in which there are various protocol encoder/decoder layers that wrap an underlying connection or individual request (not necessarily an HTTP request). Within the scope of that connection or request, user code could effectively wire up the decoder layers to each other whilst also getting direct access to whichever layers they need by simply summoning them via DI. It'd be in the end just a bit of syntax convenience - rather than taking a low level socket channel or equivalent and wrapping it in decoder layers yourself, DI would do it for you. But obviously then you'd be using mostly request scopes, which is why I went looking for it.

Fundamentally it's all here, it's just not really documented or user controlled. Instead a handful of frameworks are privileged. Nice if you use those frameworks, but it makes it a bit less flexible for experimentation and learning purposes.

@rbygrave
Copy link
Contributor

rbygrave commented Feb 3, 2021

JPMS gives the dependency graph of each module

I have not looked at that at all - it does sound like a real possibility there.

then you'd be using mostly request scopes

Ah right, hmmm.

it's just not really documented or user controlled. Instead a handful of frameworks are privileged.

So with these privileged frameworks (Javalin, Helidon, Jex) we end up with 2 things that are source generated for handling request scope. avaje-inject is generating the "factories" (instances of BeanFactory / BeanFactory2 that create/wire things given request instances as arguments. The second thing is avaje-http generates a singleton handler/adapter that knows it needs to use the avaje-inject factories to create the request scope controllers.

So with these privileged frameworks in terms of request scope we get those 2 things source generated for us (avaje-inject "factories" & avaje-http "request adapters") and both of these are detecting the "request scope nature" of the controllers.

Hmmm. I'm not sure you are going to get much joy in your scenario.

@mikehearn
Copy link
Author

OK, let's take a step back from the notion of 'request scope' which is rather HTTP specific. For example, in the hypothetical use case I gave above, it'd actually be connection scope not request scope.

If a BeanContext represents a graph of resolved objects, why can't the user just layer one on top of another? That is, there'd be the 'system context' for genuine singletons, and then a per-connection BeanScope. It appears that this is already possible anyway: it's how modules work, and it's how you're meant to override objects for testing, which is one of the big wins of DI. In fact, what makes a BeanContext a context and not a scope? Is the issue here one of docs and naming rather than actually missing functionality?

@rbygrave
Copy link
Contributor

rbygrave commented Feb 4, 2021

Yeah, I've got to "un-wind my thinking" a bit. If I look at https://github.com/avaje/avaje-inject/blob/master/inject/src/main/java/io/avaje/inject/DBeanContextBuilder.java#L104 ... and deconstruct this method a bit.

  • Obtain BeanContextFactory implementations and order them / multi-module ordering (maybe manually do this for now)
  • Have some supplied beans (maybe connection etc) and create a builder - Builder rootBuilder = Builder.newRootBuilder(suppliedBeans, enrichBeans);
  • rootBuilder.addChild(factory); .. for the factory create a BeanContext and wires constructors
  • rootBuilder.build(); ... post constructor injection (field/method injection) [after all BeanContext/modules have been constructor injected]
  • beanContext.start(); ... run post construct lifecycle

If we think of supplied beans as "beans provided" for a BeanContext scope (e.g. connections) and write some code that replaces the DBeanContextBuilder.build() method but performs the same steps I think that gets close to what you are looking for? If that is the case then perhaps it is a matter of exposing this in a better way?

@rbygrave
Copy link
Contributor

rbygrave commented Feb 5, 2021

In fact, what makes a BeanContext a context and not a scope?

Yes I think it is valid to think of BeanContext as a scope in this sense. My personal issue is that "Singleton scope" and "Request scope" are now almost wired into my brain to mean certain things in a web app/controller/jaxrs way.

My thought is that in order to treat BeanContext as a "scope" that is useful to apps that need to create a BeanContext programmatically) is for avaje-inject to provide an api that takes suppliedBeans + optionally an already existing BeanContext and creates another BeanContext.

Approximately:

// given
BeanContext parentContext = ...; // an existing BeanContext
List<SuppliedBean> suppliedBeans= ...; // contains the connection instance
BeanContextFactory factory = ...;  // the generated factory for our BeanContext/scope


// build a BeanContext "scope"
Builder builder = Builder.newRootBuilder(suppliedBeans, emptyList());

// provide existing "parent" BeanContext / layering
builder.setParent(parentContext);// *** We can't do this yet, parent is a Builder not a BeanContext !

builder.addChild(factory);

BeanContext beanContext = builder.build();
beanContext.start();  // start post construct

Perhaps :

// given
BeanContext parentContext = ...; // an existing BeanContext
Connection connection = ...; // supplied connection instance
BeanContextFactory factory = ...;  // the generated factory for our BeanContext/scope


// conceptual api to build a BeanContext "scope"
BeanContext myBeanContext = 
  BeanContext.newBuilder()
    .withBean(Connection.class, connection)

     // methods that don't exist yet
    .withParent(parentContext ) // supply a "parent" BeanContext 
    .withFactory(factory) // supply the factories to execute rather than use ServiceLoader (probably in order)
    .create();  // build and start

// and later dispose (existing)
myBeanContext.close(); 

How does that sound? Getting closer?

@mikehearn
Copy link
Author

Yeah, that looks about right. Maybe BeanContext could even be renamed BeanScope as "context" is quite a vague and overloaded term in programming. In the default case I'd use the system context/scope as the parent - perhaps it could be the default parent. The context can be auto-closeable so the DI scope can be linked to a standard stack scope.

@rbygrave
Copy link
Contributor

rbygrave commented May 13, 2021

Maybe BeanContext could even be renamed BeanScope as "context" is quite a vague

Yes. I agree and we are early enough to make this change and bump major version.
In a way SystemContext could be better named as ApplicationScope.
... so probably 2 things here.

  1. Build / expose the API as I've outlined above
  2. Rename BeanContext -> BeanScope, SystemContext -> ApplicationScope (this would come with a bump of the major version)

Extra thought:

The above works if the "request scoped beans" are effectively in a different module/jar and that could be deemed a bit limiting and confusing from the perspective that today they would have @Singleton on them (they are singleton inside the "bean scope" but maybe that is confusing and maybe we can do better).

An alternative thought is to literally have an explicit @RequestScope so that a single module (actually all modules) there are dependencies that are either like the @Singleton today or explicitly deemed "request scope" and this means that they are excluded from that default BeanScope [so not actually wired, we know about them but don't wire them yet] . In order to get these "request scoped" beans we then programmatically do something more like:

// given
// BeanScope parent =  ApplicationScope.get(); // Everything all explicitly `@Singleton`
// BeanContextFactory factory = ...;  // the generated factory for our BeanContext/scope
// ... we don't do the above which means:
// A) The ApplicationScope is our "parent".  All `@Singleton` are available to be wired to request scoped beans
// B) We don't need the factories in that ... instead `@RequestScope`beans are wired [on demand I believe]

// Things we provide to the request BeanScope for wiring
//  ... our @RequestScope can depend on these provided things + singletons + other request scope things 
Connection connection = ...; 
Pump requestPump = ....;
Pump responsePump = ....;

// conceptual api to build a request BeanScope
try (BeanScope requestScope = 
  BeanScope.newRequestScope()  // .newBuilder()
    .withBean(Connection.class, connection)
    .withBean("reqPump", Pump.class, requestPump) // with a name qualifier
    .withBean("resPump", Pump.class, responsePump)

    // ... we not longer need these bits
    //.withParent(parent)  // instead just assume ApplicationScope is parent (all Singleton's available to this scope)
    //.withFactory(factory) // instead create `@RequestScope` beans on demand rather than give supplied factories
    .create();) {  // build and start
    
   // these are created on demand
   MyRequestScopedThing thing = requestScope.get(MyRequestScopedThing.class);
   OtherRequestScopedThing other = requestScope.get(OtherRequestScopedThing.class);
   ...

  // auto disposed
  // requestScope.close(); 
}

Edit: Extra thought:
Might be nice to have BeanRequestScope & BeanRequestScopeBuilder distinct from BeanScope/BeanScopeBuilder as we have the mock and spy stuff on BeanScopeBuilder and that doesn't make sense in the request scope use case.

The context can be auto-closeable

Noting that is already is. BeanContext extends Closable (which extends AutoClosable). So we are already good on that part. It does remind of the question around if a bean implements Closable / AutoClosable if they should automatically get a PreDestroy hook (as opposed to requiring an explicit @PreDestroy like Micronaut etc). My view is that it would be better and more expected to close these beans without requiring an explicit @PreDestory. I'll create a separate ticket for this question.

@rbygrave
Copy link
Contributor

Ok, that all works and to me makes sense, we end up with:

// Mark something as a request scoped thing via @Request

@Request
MyRequestScopedThing {

  // it can depend on things provided to the request scope (e.g. Connection passed in when creating the request scope)
  // it can depend on things provided by the existing bean scope (e.g. SomeService is a @Singleton) 

  @Inject
  MyRequestScopedThing(Connection connection, SomeService service) {

  }

}

And with SystemContext renamed to ApplicationScope we get ...

// instances we provide when creating the request scope
Connection connection = ...; 
Pump requestPump = ....;
Pump responsePump = ....;


try (RequestScope requestScope = ApplicationScope.newRequestScope() 
    .withBean(Connection.class, connection)
    .withBean("reqPump", Pump.class, requestPump) // with a name qualifier
    .withBean("resPump", Pump.class, responsePump)
    .build()) { 
    
   // obtain the request scoped beans (or beans from the bean scope)
   MyRequestScopedThing thing = requestScope.get(MyRequestScopedThing.class);
   OtherRequestScopedThing other = requestScope.get(OtherRequestScopedThing.class);
   ...

   // obtain a @Singleton from the underlying bean scope
   SomeService someService = requestScope.get(SomeService.class);


  // auto dispose any closable's that were created as part of request scope
}

And we can do ApplicationScope.newRequestScope() or get a BeanScope and do beanScope.newRequestScope()

@rbygrave
Copy link
Contributor

Ok, closing this now via PR #107 . Apologies @mikehearn , very likely too late for you but I think we have a good result/approach now.

Cheers, Rob.

@rbygrave rbygrave added this to the 6.0 milestone May 16, 2021
@rbygrave rbygrave self-assigned this May 16, 2021
@mikehearn
Copy link
Author

That's no problem at all, thanks for picking this up Rob. My query originated from research that was planning out my current project, so I may well end up using this new feature soon!

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

No branches or pull requests

2 participants