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

[JBTM-3106][WFLY-11858] Fire appropriate scope events for TransactionScoped scope. Remove bean for InitialContext. #1403

Merged
merged 1 commit into from Mar 22, 2019

Conversation

@ljnelson
Copy link
Contributor

ljnelson commented Feb 11, 2019

Signed-off-by: Laird Nelson ljnelson@gmail.com

This pull request:

  1. Corrects a few generics issues in TransactionContext
  2. Introduces the DelegatingTransactionManager abstract class
  3. Introduces the NarayanaTransactionManager concrete class that extends from DelegatingTransactionManager
    1. This class fires @Initialized(TransactionScoped.class) from its begin() method
    2. This class fires @Destroyed(TransactionScoped.class) from its commit() and rollback() methods
  4. Moves JNDI detection logic from TransactionExtension into NarayanaTransactionManager

The original forum discussion behind all this is here: https://developer.jboss.org/thread/279595

https://issues.jboss.org/browse/JBTM-3106
https://issues.jboss.org/browse/WFLY-11858

Requires: jbosstm/jboss-as#80

!QA_JTA !QA_JTS_JDKORB !QA_JTS_OPENJDKORB !QA_JTS_JACORB !BLACKTIE !XTS PERF NO_WIN !RTS !TOMCAT !JACOCO !LRA

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

Copy link

jbosstm-bot commented Feb 11, 2019

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

Copy link

jbosstm-bot commented Feb 11, 2019

@jbosstm-bot

This comment has been minimized.

Copy link

jbosstm-bot commented Feb 11, 2019

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

Copy link

jbosstm-bot commented Feb 11, 2019

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@ochaloup

This comment has been minimized.

Copy link
Member

ochaloup commented Mar 21, 2019

@ljnelson ah, one more think. When you will be squashing the commits could you please prefix the commit message with both jiras this touches. I mean with JBTM-3106 and WFLY-11858 being used in the commit description?

[JBTM-3106][WFLY-11858] ... some text ...
@jbosstm-bot

This comment has been minimized.

…Scoped scope. Remove bean for InitialContext.

Signed-off-by: Laird Nelson <ljnelson@gmail.com>
@ljnelson ljnelson force-pushed the ljnelson:scope-events branch from 8d57259 to e83cef7 Mar 21, 2019
@ljnelson

This comment has been minimized.

Copy link
Contributor Author

ljnelson commented Mar 21, 2019

Hi, @ochaloup; I believe (again!) that I'm all set here.

@ljnelson ljnelson changed the title [JBTM-3106] Initial PR revision [JBTM-3106][WFLY-11858] Fire appropriate scope events for TransactionScoped scope. Remove bean for InitialContext. Mar 21, 2019
@ochaloup

This comment has been minimized.

Copy link
Member

ochaloup commented Mar 21, 2019

@ljnelson yes, it seems all good. Just let's wait for the final check from the CI.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

Copy link

jbosstm-bot commented Mar 22, 2019

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

Copy link
Contributor

tomjenkinson left a comment

LGTM - thanks for the contributions!

@tomjenkinson tomjenkinson merged commit 4b2c516 into jbosstm:master Mar 22, 2019
@mmusgrov

This comment has been minimized.

Copy link
Contributor

mmusgrov commented Mar 26, 2019

TESTIT

@ochaloup

This comment has been minimized.

Copy link
Member

ochaloup commented Mar 26, 2019

@mmusgrov I'm not sure if the TESTIT message was intentional but as the PR was already merged the CI won't run the tests for this PR.

@mmusgrov

This comment has been minimized.

Copy link
Contributor

mmusgrov commented Mar 26, 2019

@ochaloup I wasn't sure. I have just noticed that the PR disabled perf testing and since the PR wraps the TM and also generates events on a per transaction basis so I needed to check that there was no performance hit.

@ochaloup

This comment has been minimized.

Copy link
Member

ochaloup commented Apr 1, 2019

hi @ljnelson , I wanted to update our quickstarts with this new capability and I hit an issue with the transaction context deactivation.
When using @Transactional(Transactional.TxType.MANDATORY) for the whole method there is thrown exception ARJUNA016110: Transaction is required for invocation. I think the TransactionalInterceptorMandatory should probably being able to find out that the Observer is called and not being forcing the transaction to be active.
Would you have some observations about this.

The code is here:
https://github.com/ochaloup/quickstart-jbosstm/blob/JBTM-3106-testing/jta-1_2-standalone/src/main/java/org/jboss/narayana/quickstarts/jta/MandatoryCounterManager.java#L37

javax.transaction.TransactionalException: ARJUNA016110: Transaction is required for invocation
 at com.arjuna.ats.jta.cdi.transactional.TransactionalInterceptorMandatory.doIntercept(TransactionalInterceptorMandatory.java:57)
 at com.arjuna.ats.jta.cdi.transactional.TransactionalInterceptorBase.intercept(TransactionalInterceptorBase.java:88)
 at com.arjuna.ats.jta.cdi.transactional.TransactionalInterceptorMandatory.intercept(TransactionalInterceptorMandatory.java:51)
 at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
 at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
 at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
 at java.lang.reflect.Method.invoke(Method.java:498)
 at org.jboss.weld.interceptor.reader.SimpleInterceptorInvocation$SimpleMethodInvocation.invoke(SimpleInterceptorInvocation.java:73)
 at org.jboss.weld.interceptor.proxy.InterceptorMethodHandler.executeAroundInvoke(InterceptorMethodHandler.java:84)
 at org.jboss.weld.interceptor.proxy.InterceptorMethodHandler.executeInterception(InterceptorMethodHandler.java:72)
 at org.jboss.weld.interceptor.proxy.InterceptorMethodHandler.invoke(InterceptorMethodHandler.java:56)
 at org.jboss.weld.bean.proxy.CombinedInterceptorAndDecoratorStackMethodHandler.invoke(CombinedInterceptorAndDecoratorStackMethodHandler.java:79)
 at org.jboss.weld.bean.proxy.CombinedInterceptorAndDecoratorStackMethodHandler.invoke(CombinedInterceptorAndDecoratorStackMethodHandler.java:68)
 at org.jboss.narayana.quickstarts.jta.MandatoryCounterManager$Proxy$_$$_WeldSubclass.transactionScopeDectivated(Unknown Source)
@ljnelson

This comment has been minimized.

Copy link
Contributor Author

ljnelson commented Apr 1, 2019

This is a tricky one. I think the situation is—unfortunately—exactly as CDI wants it to be.

You have a method, transactionScopeDeactivated, which is effectively annotated with @Transactional(Transactional.TxType.MANDATORY). It is an observer method, which means that CDI will apply relevant interceptors, decorators, etc. So CDI begins to do this. One of the interceptors Narayana's transactional interceptor, and boom.

If we could use CDI 2.0, then you could observe the @BeforeDestroyed(TransactionScoped.class) qualifier, and this method could be notified right before the transaction is deactivated.

In summary, I think that a MANDATORY method that observes the (post-) destruction of the transaction scope is nonsensical, so this exception doesn't surprise me.

@ochaloup

This comment has been minimized.

Copy link
Member

ochaloup commented Apr 1, 2019

@ljnelson I see, thanks for input. I was thinking if the UX would be better of making the interceptor being aware of an observer methods. But as you say it's the expected behaviour and ok for how the CDI works.

Good point about the @BeforeDestroyed. Thanks for the mentioning it.

@ljnelson

This comment has been minimized.

Copy link
Contributor Author

ljnelson commented Apr 1, 2019

Oh, I see what you're asking, I think: if you're asking, "Should we change the interceptors to be aware of these observer methods in some way?" I would say no, don't do that.

Also you'll note that the NarayanaTransactionManager fires a String as the event qualified with @Destroyed(TransactionScoped.class) on purpose instead of a Transaction object for similar reasons.

@ochaloup

This comment has been minimized.

Copy link
Member

ochaloup commented Apr 1, 2019

@ljnelson yeap, ok, thanks.

I already wondered about the fact that the String is fired instead of the Transaction. But then I found the reason about the empty (null) transaction context.

@ljnelson

This comment has been minimized.

Copy link
Contributor Author

ljnelson commented Apr 1, 2019

Right; the point being that really observers should just be observing Object, since it doesn't really matter what the payload is. Additionally, Weld itself fires String in several built-in scope-destruction situations.

@ochaloup

This comment has been minimized.

Copy link
Member

ochaloup commented Apr 3, 2019

@ljnelson thanks for the reply!

I have another doubt ojn my mind. I wanted to run the CDI code without need to define InitialContext() binding. My understanding was that's the idea of your fixes at the end. And I liked that idea.

I want to get running standalone Java SE example. I did new Weld().initialize() (https://github.com/ochaloup/quickstart-jbosstm/blob/JBTM-3106-testing-lookup-failure/jta-1_2-standalone/src/test/java/org/jboss/narayana/quickstarts/jta/CDIBindingTestCase.java#L49). But that's where the error[1] happens.

The reason is that the JNDIBean (https://github.com/jbosstm/narayana/blob/5.9.5.Final/ArjunaJTA/cdi/classes/com/arjuna/ats/jta/cdi/JNDIBean.java#L122) and in the same manner the NarayanaTransactionManager (https://github.com/jbosstm/narayana/blob/5.9.5.Final/ArjunaJTA/cdi/classes/com/arjuna/ats/jta/cdi/NarayanaTransactionManager.java#L183) catches the NamingException and throws that up. Even the NarayanaTransactionManager defines fallback to com.arjuna.ats.jta.TransactionManager.transactionManager() (https://github.com/jbosstm/narayana/blob/5.9.5.Final/ArjunaJTA/cdi/classes/com/arjuna/ats/jta/cdi/NarayanaTransactionManager.java#L204) which I would be happy to be used in my example it's never used. The InitialContext.lookup() ends with the NamingException either if there is connection timeout (aka. JNDI server is defined but is not started) or if no JNDI context exists[2]. In both cases the NamingException causes the code just ends with error and do not use the defined fallback.

Please @ljnelson , do you have some code using this approach and can you point me to reason this does not work? Or can I consider it rather as an issue of the current implementation which should be adjusted? (which I consider more probable).

Thanks a lot
o.

[1]

testTransactionScoped(org.jboss.narayana.quickstarts.jta.CDIBindingTestCase)  Time elapsed: 5.879 sec  <<< ERROR!
javax.enterprise.inject.CreationException: Need to specify class name in environment or system property, or as an applet parameter, or in an application resource file:  java.naming.factory.initial
 at com.arjuna.ats.jta.cdi.JNDIBean.create(JNDIBean.java:123)
 at org.jboss.weld.contexts.AbstractContext.get(AbstractContext.java:96)
 at org.jboss.weld.bean.ContextualInstanceStrategy$DefaultContextualInstanceStrategy.get(ContextualInstanceStrategy.java:100)
 at org.jboss.weld.bean.ContextualInstance.get(ContextualInstance.java:50)
 at org.jboss.weld.manager.BeanManagerImpl.getReference(BeanManagerImpl.java:694)
 at org.jboss.weld.manager.BeanManagerImpl.getReference(BeanManagerImpl.java:717)
 at org.jboss.weld.util.ForwardingBeanManager.getReference(ForwardingBeanManager.java:64)
 at org.jboss.weld.bean.builtin.BeanManagerProxy.getReference(BeanManagerProxy.java:87)
 at com.arjuna.ats.jta.cdi.TransactionExtension.lambda$afterBeanDiscovery$1(TransactionExtension.java:107)
 at com.arjuna.ats.jta.cdi.TransactionContext.get(TransactionContext.java:107)
 at com.arjuna.ats.jta.cdi.TransactionContext.get(TransactionContext.java:139)
 at org.jboss.weld.contexts.PassivatingContextWrapper$AbstractPassivatingContextWrapper.get(PassivatingContextWrapper.java:78)
 at org.jboss.weld.bean.ContextualInstanceStrategy$DefaultContextualInstanceStrategy.getIfExists(ContextualInstanceStrategy.java:89)
 at org.jboss.weld.bean.ContextualInstance.getIfExists(ContextualInstance.java:63)
 at org.jboss.weld.bean.proxy.ContextBeanInstance.getInstance(ContextBeanInstance.java:87)
 at org.jboss.weld.bean.proxy.ProxyMethodHandler.getInstance(ProxyMethodHandler.java:131)
 at org.jboss.narayana.quickstarts.jta.Counter$Proxy$_$$_WeldClientProxy.get(Unknown Source)
 at org.jboss.narayana.quickstarts.jta.RequiredCounterManager.getCounter(RequiredCounterManager.java:75)
 at org.jboss.narayana.quickstarts.jta.RequiredCounterManager$Proxy$_$$_WeldSubclass.getCounter(Unknown Source)
 at org.jboss.narayana.quickstarts.jta.CDIBindingTestCase.testTransactionScoped(CDIBindingTestCase.java:78)
 at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
 ...
Caused by: javax.naming.NoInitialContextException: Need to specify class name in environment or system property, or as an applet parameter, or in an application resource file:  java.naming.factory.initial
 at javax.naming.spi.NamingManager.getInitialContext(NamingManager.java:662)
 at javax.naming.InitialContext.getDefaultInitCtx(InitialContext.java:313)
 at javax.naming.InitialContext.getURLOrDefaultInitCtx(InitialContext.java:350)
 at javax.naming.InitialContext.lookup(InitialContext.java:417)
 at com.arjuna.ats.jta.cdi.JNDIBean.create(JNDIBean.java:121)
 ... 50 more

[2] InitialContext.lookup failure

Exception in thread "main" java.lang.RuntimeException: javax.naming.NoInitialContextException: Need to specify class name in environment or system property, or as an applet parameter, or in an application resource file:  java.naming.factory.initial
 at org.jboss.narayana.quickstarts.jta.MyMain.main(MyMain.java:11)
Caused by: javax.naming.NoInitialContextException: Need to specify class name in environment or system property, or as an applet parameter, or in an application resource file:  java.naming.factory.initial
 at javax.naming.spi.NamingManager.getInitialContext(NamingManager.java:662)
 at javax.naming.InitialContext.getDefaultInitCtx(InitialContext.java:313)
 at javax.naming.InitialContext.getURLOrDefaultInitCtx(InitialContext.java:350)
 at javax.naming.InitialContext.lookup(InitialContext.java:417)
 at org.jboss.narayana.quickstarts.jta.MyMain.main(MyMain.java:9)
@ljnelson

This comment has been minimized.

Copy link
Contributor Author

ljnelson commented Apr 3, 2019

I'll look later today. I am pretty sure that it is a simple oversight: https://github.com/jbosstm/narayana/blob/5.9.5.Final/ArjunaJTA/cdi/classes/com/arjuna/ats/jta/cdi/NarayanaTransactionManager.java#L183 catches NoInitialContextException, whereas https://github.com/jbosstm/narayana/blob/5.9.5.Final/ArjunaJTA/cdi/classes/com/arjuna/ats/jta/cdi/JNDIBean.java#L122 does not, and probably should.

In my own work I actually supply my own TransactionSynchronizationRegistry bean, so the code block that creates the JNDIBean never executes.

@ljnelson ljnelson deleted the ljnelson:scope-events branch Apr 3, 2019
@ljnelson

This comment has been minimized.

Copy link
Contributor Author

ljnelson commented Apr 3, 2019

Hi, @ochaloup; I filed https://issues.jboss.org/browse/JBTM-3131 to track this; I'll have a PR shortly.

@ochaloup

This comment has been minimized.

Copy link
Member

ochaloup commented Apr 4, 2019

@ljnelson thanks a lot! I will check it out with my code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.