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.

Sign up for GitHub

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

Jump to bottom

Deprecate @Context annotations #639

Closed
ggam opened this issue Jul 23, 2018 · 77 comments
Closed

Deprecate @Context annotations #639

ggam opened this issue Jul 23, 2018 · 77 comments
Assignees
@spericas
Labels
api enhancement New feature or request
Milestone
3.1

Comments

@ggam
Copy link
Contributor

ggam commented Jul 23, 2018 •
edited
Loading

Once we have a complete CDI replacement for @Context annotations (tentatively on 3.0), they should be deprecated as a first step before removal on a later release.
@arjantijms
Copy link
Contributor

@ggam isn't that replacement already available in the form of @Inject?

@mkarg mkarg added this to the 3 milestone Jul 23, 2018
@mkarg mkarg added enhancement New feature or request api labels Jul 23, 2018
@ggam
Copy link
Contributor Author

ggam commented Jul 23, 2018 •
edited
Loading

@arjantijms I'm actually with you. I think @Inject could deprecate @Context even on 2.2, but I think there's some consensus that 2.2 is too early.

This issue is just to trigger the needed discussions to make it happen as soon as possible. For me that would be 2.2, but we need to get @path as a bean defining annotation (perhaps via a @Stereotype), which sadly I don't see done for 2.2.

@arjantijms
Copy link
Contributor

Thanks @ggam

I'm not really sure that there is much consensus that 2.2 is too early, especially when we have no idea yet when we're even able to release a 2.2.

Having @Path being bean defining or effectively causing beans with that annotation to be scanned is more of a nice to have than a big requirement IMHO. Might be worth it though to double check how this is done in the MVC spec with @chkal.

The reason why I'm pushing this a little is that I've seen how always thinking that it's too early has delayed this by many years in JSF. The native managed beans could already have been replaced by CDI in JSF 2.0 (2009), and when that window of opportunity was missed surely in JSF 2.2 (2013). But people kept being afraid that it was "too soon", meanwhile causing quite a bit of confusion. I'm fairly sure that if I didn't finally deprecate managed beans in JSF 2.3, they still wouldn't be deprecated today.

Although I think @Path doesn't have to be bean defining for a 2.2 release, it might be quite doable to have beans with the @Path annotation scanned by making use of a CDI extension, or indeed a stereo type.

@mkarg
Copy link
Contributor

mkarg commented Jul 23, 2018 •
edited
Loading

The point with 2.2 is that CDI is optional still in that release. If you want to deprecate @Context in favor of @Inject, this implies that you want to deprecate non-CDI in favor of CDI. It is a paradoxon to do that without making CDI mandatory.

@arjantijms
Copy link
Contributor

The idea is to have @Context formally deprecated by actually putting an @Deprecated annotation on it, and then using an @deprecated annotation in Javadoc pointing to the usage of CDI.

In that case, of course, CDI MUST be supported by the implementations. User code however that doesn't use CDI (yet) can still run unchanged, as @Context, even though being deprecated still works.

@Context can then be actually removed (pruned) in 3.0.

@ggam
Copy link
Contributor Author

ggam commented Jul 23, 2018 via email

@mkarg
Copy link
Contributor

mkarg commented Jul 24, 2018 via email

@mkarg
Copy link
Contributor

mkarg commented Jul 24, 2018 via email •
edited
Loading

@spericas
Copy link
Contributor

We had some earlier discussions about this already. Even if we assume that all implementations support CDI (as stated in the spec today), the transition from @Context to @Inject is not as straightforward. That is, if a developer gets a deprecated message about @Context, it may take more than replacing it by @Inject for the application to work.

As I argued before, we need to have a recipe that explains how to transition from one DI to another, and this is more than a simple annotation replacement:

  1. @Context in param position
  2. @Context with ContextResolvers
  3. ContextResolvers to configuration implementations (JAXB etc)
  4. Using @Inject for JAX-RS types like UriInfo, SecurityContext

And there's likely more.

@arjantijms
Copy link
Contributor

So if you want to follow that plan (which I second), please set up an official vote, so we have confidence.

vs

So just +1'ing on the API project does not change much, unfortunately.

Am I the only one who sees some potentially conflicting statements there? If they're not conflicting, please explain ;) It's not a bad idea to start a vote about this.

Anyway, Payara, which uses Jersey, and to which we contribute, is in favour of CDI support in JAX-RS. If I'm not mistaken we have mentioned this before.

There's not that much for Jersey to change there anyway, as it supports CDI already today.

@arjantijms
Copy link
Contributor

As I argued before, we need to have a recipe that explains how to transition from one DI to another, and this is more than a simple annotation replacement:

@context in param position
@context with ContextResolvers
ContextResolvers to configuration implementations (JAXB etc)
Using @Inject for JAX-RS types like UriInfo, SecurityContext
And there's likely more.

I agree, so it's a case of carefully analysing each of those.

There's a few things I'm sure we can do regardless, even when we ultimately decide not to use the @Deprecate annotation just yet.

For instance, saying that implementations MUST support CDI is quite close to what the spec already asks. I have to look up the exact wording, maybe it's even already fine as it is.

Adding built-in beans for JAX-RS types like UriInfo, SecurityContext is probably quite doable and doesn't break anything else. It would just mean that one can use either @Context or @Inject. Shall I create a separate issue just for that?

@mkarg
Copy link
Contributor

mkarg commented Jul 24, 2018

@arjantijms What I wanted to tell with that statements is that I would be happy to have an official vote resulting in the fact that the majority of the committers wants to make CDI mandatory in 3.0, but that we have to understand that still all vendors have to implement CDI if not done already. For Jersey, CDI is done in part (not completely: at least not for the standalone server backends but only for the containers like used in Payara; and you cannot use an already existing SeContainerInitializer which is needed once you run standalone without Payara). Anyways, a standard is only good if more than one product supports them, and until yet, nobody besides Jersey spoke up here.

@mkarg
Copy link
Contributor

mkarg commented Jul 24, 2018

There's a few things I'm sure we can do regardless, even when we ultimately decide not to use the @deprecate annotation just yet.

I agree, but this issue discusses explicitly @Deprecate, so all the rest of the discussion has to go in its own issue. :-)

@spericas
Copy link
Contributor

@arjantijms I think rather than creating more issues, and then having these isolated discussions started again, we should create a wiki and analyze all the @Context use cases and propose alternatives using CDI machinery. From that discussion, we should be able to create additional requirements for the spec. Any volunteers?

@mkarg
Copy link
Contributor

mkarg commented Jul 24, 2018 •
edited
Loading

@arjantijms Maybe you simply like to open a PR that shows us how easy and safe all your ideas fit into JAX-RS 2.2? So we know exactly what you do propose. BTW, the exact words of the spec are: 11.2.3 Context and Dependency Injection (CDI) In a product that supports CDI, implementations MUST support the use of CDI-style Beans as root resource classes, providers and Application subclasses. Providers and Application subclasses MUST be singletons or use application scope. . That's all. Nothing about "MUST".

@arjantijms
Copy link
Contributor

@arjantijms I think rather than creating more issues, and then having these isolated discussions started again, we should create a wiki and analyze all the @context use cases and propose alternatives using CDI machinery. From that discussion, we should be able to create additional requirements for the spec. Any volunteers?

It's true that we don't want too many isolated discussions where the same things are repeated. Definitely +1 for that.

On the other hand, there's the well known divide and conquer tactics, which of course everyone here knows about it.

Fully deprecating @Context is, as you mentioned, a potentially bigger task than it seems. Just doing a single part of it that can be done regardless of the other difficulties and has value by itself, may not hurt there.

I'll start an initial wiki page then based on your list presented here, and will update that with some of discussions we already had about it.

@mkarg
Copy link
Contributor

mkarg commented Jul 24, 2018

Great, thanks! Until then, I assume we will keep my initial setting that this issue has to stay with 3.0 and cannot be done earlier, just as I said from the start. ;-)

@arjantijms
Copy link
Contributor

@mkarg

@arjantijms Maybe you simply like to open a PR that shows us how easy and safe all your ideas fit into JAX-RS 2.2? So we know exactly what you do propose.

A PR for CDI injectable beans which are owned by JAX-RS (so that they don't need a qualifier), probably doesn't have much or any API surface, and I assume it will be mostly an implementation thing (with just a mention in the spec document, see for example how the CDI core spec defines that HttpServletRequest is injectable)

@mkarg
Copy link
Contributor

mkarg commented Jul 24, 2018

@arjantijms Sad but true, this one more proofs that Oracle & EF really have to find an agreement soon regarding the spec documents. We simple need them ASAP.

@asoldano
Copy link

@arjantijms @spericas +1 on creating the wiki page to deal with all @context usecases, thanks

@jansupol
Copy link
Contributor

AFAIK, the Eclipse Foundation has a process that recommends a release once per 3 months (did not check). Should the 2.2 release take place in autumn and 3.0 in mid 19, I see a room for 1 - 2 additional JAXRS releases that can actually deprecate @context (at that time the impls can have both @context and @Inject working). I do not see an urge to do so in 2.2.

@arjantijms
Copy link
Contributor

I see a room for 1 - 2 additional JAXRS releases that can actually deprecate @context (at that time the impls can have both @context and @Inject working). I do not see an urge to do so in 2.2.

Given that we indeed go for 1 or 2 additional releases (say 2.3, 2.4), then I agree and we can do it in 2.3 if needed.

My main point is we can start working on it now, and whether it will be included in 2.2 or 2.3 is another question. I don't see much need to wait (e.g. until 3.0) just for the sake of waiting, especially if it's not clear what we'd be exactly waiting for.

@mkarg
Copy link
Contributor

mkarg commented Jul 26, 2018

Yes, you can start working on it now, and depending on when it is finished and how much breaking it really is we can decide then into which development stream (next major or next minor) we have to merge it. Until then, the only realistic option is to deprecate in 3.0. Hence, again, this issue #639) has to be on hold for the time being.

@FroMage
Copy link

FroMage commented Oct 10, 2018

Last time I checked, CDI did not support injecting in method parameters, where @Context had to be used. Did that change?

@FroMage
Copy link

FroMage commented Oct 10, 2018 •
edited
Loading

Sorry I just now realised that was discussed already here and in #569. My personal opinion is that method param injection alone is a good reason to keep @Context because moving to a field injection is more verbose, less local, and potentially more costly because not every method need every injected field, not to mention more messy with variable proliferation.

As an added benefit we have with @Context: in RESTEasy we were able to implement pluggable async injection, where producers of async types (such as CompletionStage but extensible to RxJava and others) can be first resolved to their actual value (asynchronously) and only then injected in the resource. CDI does not support async injection ATM, and it's pretty much trivial to implement in JAX-RS implementations with @Context. Dropping this means less rapid innovation too.

@arjantijms
Copy link
Contributor

There’s no reason to keep @context for method parameter injection, and also no reason to switch to field injection when using CDI.

The JAX-RS runtime can easily inject values into a method using CDI, as explained before.

@FroMage
Copy link

FroMage commented Oct 11, 2018

Well, yes we can do the injection ourselves for method parameters, but I'm not sure we can do async injection for fields anymore if CDI is in charge. I'd have to check that.

@arjantijms
Copy link
Contributor

I imagine his usecase accounts for the injcetion of e.g. an
EntityManager that obtains the connection from a pool and possibly has to
wait for some connection to be free.

But how often does one uses an entity manager in the @PostContruct method?

The injection of just the entity manager doesn't do anything yet. It's a very lightweight object.

For example:

@RequestScoped
public class SomeService {
    @Inject
     EntityManager entityManager;

      public List<User> getAll() {
          return entityManager.createNamedQuery("User.getAll", User.class)
                            .getResultList();
    }
}

When you're doing @Inject SomeService someService somewhere else, there's no connection to be allocated yet.

It depends on whether someService is actually used, but if it is, a connection is only accessed when the call to getResultList() happens.

@arjantijms
Copy link
Contributor

So this is really about treating CompletionStage producers in CDI in a
special way in order to do non blocking injection, and not about JAX-RS
producing an async response.

The injection itself would rarely be blocking. Only if it was a dependent scoped bean, that performs logic involving an (external) other system in its @PostConstruct method would that happen.

@arjantijms
Copy link
Contributor

arjantijms commented Oct 12, 2018 •
edited
Loading

It's one of the RESTEasy reactive features that we did not think to bring to the spec at this point

So if it does not depend on the spec, you can just use the same RESTEasy internals for it that @Context in RESTEasy now depends upon, can't you?

@FroMage
Copy link

FroMage commented Oct 12, 2018

@ggam yes, except that CDI would have to change API to support it: it couldn't be doing Object get() to get the resource, but would need to be non-blocking async: CompletionStage<Object> get() to support async injection. And then the container would still have to figure out if there's async injection and if yes turn the connection async like it does for resource methods that return CompletionStage so there's still things to be done in JAX-RS even if CDI ends up supporting it.

@FroMage
Copy link

FroMage commented Oct 12, 2018

@arjantijms in the reactive world there are lots of things you want injected that might actually block (anything that might hit the network).

So if it does not depend on the spec, you can just use the same RESTEasy internals for it that @context in RESTEasy now depends upon, doesn't it?

It works right now, but only if you don't remove @Context, or at least if you don't remove the possibility of JAX-RS injection.

@arjantijms
Copy link
Contributor

in the reactive world there are lots of things you want injected that might actually block (anything that might hit the network).

I understand, but what I mean is that during injection it would be awkward at best if a dependency would start hitting the network. There's really only two things potentially executed when a dependent s scoped dependency is injected during injection time; the constructor and the @PostConstruct method.

Accessing the network from those methods is not super common I think (of course, it's possible).

But, injecting dependent scoped objects themselves is already relatively rare. If you inject normal scoped, there's nothing executed during injection time at all, as you get a lightweight proxy.

The resources that such dependency uses are typically prepared way upfront. E.g. connection are allocated during startup of the server and put into a connection pool.

I do see why you'd want to have an instance injected that has eagerly resolved all of its potentially heavyweight (custom) dependencies, but as I mentioned, constantly eagerly instantiating has its costs too if the dependency is not always actually used.

@ggam
Copy link
Contributor Author

ggam commented Oct 12, 2018 via email

@ggam
Copy link
Contributor Author

ggam commented Oct 12, 2018 via email

@spericas
Copy link
Contributor

it would be the responsibility of service_A to define its resource method using @asynchronous and registering callbacks so that a response is only produced when B and C become available (likely on a different thread because of @asynchronous).

Except we're not relying on @Asynchronous because the injected value is produced as a CompletionStage so it's up to the producer to produce it in any non-blocking way it wants. It can really be produced by the current thread after it has registered its listeners and made the request asynchronous.

Right, understood.

have the framework change the method to asynchronous (a bit of magic) and wait for the CompletionStage's to complete before resuming execution (a bit more of magic)

The exact same amount of magic that does this for resource methods that return a CompletionStage in JAX-RS 2.1 :)

Exactly, and without looking at an example (do you have one?), if feels that the potential code simplification provided by async injection is minimal over the standard JAX-RS 2.1 API.

As you said, this is a bit off-topic, but these are the type of discussions that help improve specifications, so definitely worth having them ;)

@arjantijms
Copy link
Contributor

but before using it, the producer code will need to be
invoked. I personally don't consider producers with network dependencies a
bad practice.

Absolutely correct, but then we do come back to the question whether the injected bean will indeed be used.

Just an example:

@RequestScoped
public void SomeThing {

    @Inject 
    UserService service;

    public void persistUser(SomeParam param) {
        if (param == ... && foo == ...) {
             service.persist(...);
         }
    }
}

In that case, you will not always actually hit the UserService. So in the example of FroMage, SomeThing would be asynchronously instantiated, and all its dependencies, which is UserService here cascade into being asynchronously instantiated as well, and its dependencies too, etc.

Potentially a lot of work.

If you use a dependency all the time, or most of the time it could be worth it. If not, it's a wasted effort.

@mkarg
Copy link
Contributor

mkarg commented Oct 12, 2018

Instead of trying to convince us to keep @Context wouldn't it be a better idea to convince the CDI team to support this scenario?

@ggam
Copy link
Contributor Author

ggam commented Oct 12, 2018 via email

@FroMage
Copy link

FroMage commented Oct 15, 2018

If you use a dependency all the time, or most of the time it could be worth it. If not, it's a wasted effort.

Our usage samples reveal that method-injection (of async types) helped a lot for readability for methods where those async types where always used, especially when there were several of them.

@FroMage
Copy link

FroMage commented Oct 15, 2018

Instead of trying to convince us to keep @context wouldn't it be a better idea to convince the CDI team to support this scenario?

That's also an effort underway, but I had to point out that until this is resolved in CDI, @Context is very useful for reactive applications.

@spericas
Copy link
Contributor

If you use a dependency all the time, or most of the time it could be worth it. If not, it's a wasted effort.

Our usage samples reveal that method-injection (of async types) helped a lot for readability for methods where those async types where always used, especially when there were several of them.

Show us the samples!! :-)

@chkal
Copy link
Contributor

chkal commented Nov 8, 2018

I just wanted to share an interesting discussion about the JAX-RS <-> CDI integration from the MicroProfile mailing list. Especially, because it lists some important edge cases.

@andymc12
Copy link
Contributor

Hi All,

Related (only partially) to the topic of async injection, would we need some sort of CDI thread context propagation mechanism? I bring this up due to some discussion in the MP Concurrency about whether they should support JAX-RS specific thread context propagation, or just rely on CDI propagation - see here for more details of the MP discussion.

I envision a scenario like this:

@Context // to be replaced with @Inject
HttpHeaders headers;

@GET
public void myResourceMethod(@Suspended AsyncResponse ar) {
    ExecutorService executor = getSomeManagedExecutorService();
    executor.submit( () -> {
        String myHeader = headers.getHeaderString("AnImportantHeader"); // <-- what do we get back here?
        ar.resume( processBasedOnHeader(myHeader) );
    });
}

If we assume that HttpHeaders is request scoped, then this works just fine for synchronous resource methods (we get the header value for the request), but what about asynchronous methods like the above? The call to HttpHeaders is on another thread, so CDI won't know that it is associated with the same request from the initial thread.

Today @Context works because the JAX-RS implementations are responsible for making it work, but I'm not sure how those implementations would make this work if CDI is responsible for managing the injection - how will CDI know which HttpHeader proxy is associated with which requests?

Thanks, Andy

@sdaschner sdaschner mentioned this issue Mar 15, 2019
Open
@spericas
Copy link
Contributor

Thanks for bringing this up @andymc12. Clearly, all this works fine in the synchronous case. For async, I see two separate cases: the ExecutorService is under the JAX-RS implementation control or it isn't.

If the JAX-RS implementation does not manage the executor service, then I don't see how the code above would work even if you use @Context (as a side note, Jersey has an annotation called ManagedAsync for this purpose I believe). Does it really work?

If the JAX-RS implementation does manage the executor service, then it can make sure the original "request scope" is available when the new thread runs, and a similar mechanism would be required for CDI --this is all handwavy, there may be difficult technical details that need to be resolved for this to happen.

@FroMage
Copy link

FroMage commented Apr 26, 2019

@andymc12 note that using MP-Context-Propagation (formerly MP-Concurrency) this works with RESTEasy because it defines a ThreadContextProvider for the state it needs for @Context to work. We believe this is enough.

@spericas spericas modified the milestones: 4.0, 3.1 Apr 12, 2021
@spericas spericas self-assigned this May 4, 2021
@spericas
Copy link
Contributor

Lots of interesting discussions here, but the original purpose of the issue was about deprecation of @Context. This was done in the spec document already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@spericas spericas

Labels
api enhancement New feature or request
Projects
None yet
Milestone
3.1
Development

No branches or pull requests
10 participants
@chkal @FroMage @derekm @asoldano @mkarg @ggam @arjantijms @spericas @jansupol @andymc12