Avoiding JNDI lookups in Narayana JTA

[ljnelson]

I'm looking to enable the usage of the Narayana transactional interceptors in CDI 2.0 SE with no JNDI involvement, and also the usage of the TransactionContext class in Narayana JTA's CDI integration without the usage of JNDI.

At a high level I can accomplish this very easily if:

• TransactionContext's affiliated TransactionManager can be supplied to it rather than looked up.
• TransactionContext's affiliated TransactionSynchronizationRegistry can be supplied to it rather than looked up.
• TransactionalInterceptorBase's affiliated TransactionManager can be supplied to it rather than looked up.

Then I can make a CDI bean that creates a TransactionManager by using the com.arjuna.ats.jta.TransactionManager#transactionManager() method.

(At an even higher level, I guess I'm indicating that some of these classes could be made more DI-friendly in general.)

To contribute to this process, I filed Made getTransactionManager() protected instead of private by ljnelson · Pull Request #1344 · jbosstm/narayana · GitHub.  This was closed and it was requested that I start a discussion here so…here I am.  :-)

At the moment I have this all working with some reflection/setAccessible(true) hackery so I'm not blocked.

[tomjenkinson]

Thanks for opening the thread, it is easier to discuss here and will get more visibility.

I was wondering what is wrong with using:

> TransactionManager tm = com.arjuna.ats.jta.TransactionManager.transactionManager();

That method doesn't use JNDI.

There should only be one TransactionManager instance in the JVM so I am not sure what would be wrong with doing it that way?

I am not sure what is achieved by a reference from the TransactionContext to the same instance, can you explain further please?

[ljnelson]

Let's start from something that looks like the beginning.  I'll describe one of the scenarios I've encountered; there are others, but they follow the same pattern.

Narayana has a TransactionContext class, which supports the javax.transaction.TransactionScoped annotation.  It appears that in a CDI application I'm supposed to be able to do:

@TransactionScoped

public class Frobnicator {

}

…and now CDI will contain a bean whose bean types include Frobnicator.class and Object.class in TransactionScoped scope.

When it comes time for CDI to produce an instance of this bean, it will use the TransactionContext to acquire the contextual reference.

When it does so, TransactionContext as it is written today will (eventually) call JNDI to look up a javax.transaction.TransactionManager.  In an environment without JNDI, this fails.

I want to take JNDI out of the picture.

At the moment, I have no way to avoid a JNDI lookup.  TransactionContext will do a JNDI lookup as part of its private getTransactionManager() implementation.  This private method will yield a TransactionManager that the implementation of the get() method will then use indirectly to do its job.

What if I could change this class in some way to not use JNDI?

Let's look at the innards of TransactionContext.  It has a static transactionManager field.  On the first invocation of the private getTransactionManager() method, that static field is populated once, for all time, with the results of the JNDI lookup.  This is the field whose value is returned by TransactionContext#getTransactionManager().

Now, clearly since this field is static and populated once there is no expectation on the part of any Narayana machinery that the TransactionManager is ever going to change.  That is, the TransactionManager in JNDI is looked up once, then stored forever more in this static field.  Therefore in terms of lifecycle it is basically a singleton.

OK, fine.  Bearing in mind the ultimate goal is to get rid of JNDI, let's look at how this effectively-singleton TransactionManager gets into JNDI in the first place.  Obviously it can get in there via a number of mechanisms, but elsewhere in the Narayana codebase one way that it gets in there can be found in the com.arjuna.ats.jta.utils.JNDIManager class.  If we look in there, we find this method:

public static void bindJTATransactionManagerImplementation(InitialContext initialContext) throws javax.naming.NamingException

{

        /** Look up and instantiate an instance of the configured transaction manager implementation **/

        String tmImplementation = jtaPropertyManager.getJTAEnvironmentBean().getTransactionManagerClassName();

        /** Bind the transaction manager to the appropriate JNDI context **/

        Reference ref = new Reference(tmImplementation, tmImplementation, null);

        initialContext.rebind(getTransactionManagerJNDIName(), ref);

}

If we go spelunking and find the JTAEnvironmentBean, we can see that the default implementation to be used is com.arjuna.ats.internal.jta.transaction.arjunacore.TransactionManagerImple.

So it appears that at least in some scenarios it is perfectly valid to have an effectively-singleton instance of TransactionManagerImple (the class bound to JNDI here, and also instantiated via the same mechanism by the com.arjuna.ats.jta.TransactionManager#transactionManager() method).

So if TransactionContext could be supplied with a singleton TransactionManager, then TransactionContext would have no need of JNDI.

I proved this out by—at CDI container startup time—getting reflective access to that private static transactionManager field, and setting it to a new com.arjuna.ats.internal.jta.transaction.arjunacore.TransactionManagerImple instance before TransactionContext is ever instantiated.  Sure enough that lets TransactionContext work, once instantiated, without JNDI: the transactionManager field is non-null on first access, so JNDI access is bypassed.

To avoid this reflective hackery, there could be one of two easy changes:

1. If for some reason you want to keep TransactionContext#getTransactionManager() exactly as it is and want to keep TransactionContext as unchanged as possible, then we could make the getTransactionManager() method protected instead of private.  That at least gives an extension point for those of us who feel that JNDI doesn't belong in this class.  This would work, and be very simple, but is kind of a band-aid.
2. Change TransactionContext to not use JNDI at all anywhere by:
   1. changing its sole constructor signature to be public TransactionContext(TransactionManager tm);
   2. making transactionManager non-static and final and assigning it to tm
   3. changing getTransactionManager() to simply return this.transactionManager

This general pattern also applies to the TransactionSynchronizationRegistry used by this class, but let's just discuss this one instance of the general pattern.

A further discussion—virtually identical in its contents—can take place for similar concerns in TransactionalInterceptorBase, but, again, let's just talk about TransactionContext here.

[ochaloup]

Hi jnelson,

when considered your second suggestion, which seems nicer to me, how do you expect the extension registration (narayana/TransactionExtension.java at master · jbosstm/narayana ) will be transformed? Do you consider using JNDI neither there? Or do you expect to register/extend the context on your own then?

I feel I miss the piece here. Would you be so kind and elaborate a bit on your approach?

Thank you in advance

Ondra

[tomjenkinson]

Thanks for the indepth response. I think this would help: Comparing jbosstm:master...tomjenkinson:jndicdi · jbosstm/narayana · GitHub

It was kind of where I was going with suggesting using:

> TransactionManager.transactionManager()

But in fact I realised that that method is pretty simple and so I thought we might as well just use the same code as it does to simplify further.

[ochaloup]

tomjenkinson I would like to mention that the approach with the direct accessing the transaction manager via narayana mbean could not work in WFLY. Because of the abstraction layer provided by the wildfly transaction client there is need the JNDI to be used, I assume.

[tomjenkinson]

Ah yes, good point. Perhaps we should provide the code I in the diff, and also make the method protected and find some way to replace the TransactionContext in WFLY with one that can resolve using JNDI?

[ljnelson]

ochaloup  wrote:

Hi jnelson ,

when considered your second suggestion, which seems nicer to me, how do you expect the extension registration (narayana/TransactionExtension.java at master · jbosstm/narayana ) will be transformed? Do you consider using JNDI neither there? Or do you expect to register/extend the context on your own then?

I feel I miss the piece here. Would you be so kind and elaborate a bit on your approach?

Thank you in advance

Ondra

For my use case (CDI SE), here is the simplest way I would do it (assuming a patched TransactionContext that accepts a TransactionManager in its constructor):

public void afterBeanDiscovery(@Observes AfterBeanDiscovery event, BeanManager manager) {

  event.addContext(new TransactionContext(com.arjuna.ats.jta.TransactionManager.transactionManager()));

}

That is the shortest way I can think of to explain what I mean by "DI-friendly".  The point is that no code inside ArjunaJTA (IMHO) should be looking to a global variable store for the TransactionManager (even if the global variable store is JNDI).  Instead, a TransactionManager should be supplied to all components in ArjunaJTA that need one.

In reality I would probably get slightly more complicated:

1. I would probably change TransactionContext to take a Supplier<TransactionManager> or Provider<TransactionManager> in its constructor
2. I would arrange for a CDI bean of type TransactionManager and Singleton (not ApplicationScoped) scope to be added to the container whose create() method would return the value of invoking com.arjuna.ats.jta.TransactionManager.transactionManager()
3. I would write a Supplier<TransactionManager> or Provider<TransactionManager> implementation that would do something like CDI.current().select(TransactionManager.class).get() and then do event.addContext(new TransactionContext(mySupplierImplementation));

Best,

Laird

[ljnelson]

tomjenkinson  wrote:

Thanks for the indepth response. I think this would help: Comparing jbosstm:master...tomjenkinson:jndicdi · jbosstm/narayana · GitHub

It was kind of where I was going with suggesting using:

> TransactionManager.transactionManager()

But in fact I realised that that method is pretty simple and so I thought we might as well just use the same code as it does to simplify further.

While this would of course work, it still locates logic for acquiring a TransactionManager inside the thing that needs to use the TransactionManager.  It's far less couply :-) to simply supply the thing that needs a TransactionManager with a TransactionManager upon its creation.

Obviously I'll take anything I can get, but if we're talking about changing source code I'd rather see it changed to be more DI-friendly.

I'll put together another pull request that incorporates my reply to ochaloup.  I think it is easier to simply write it than to explain it.  :-)

Best,

Laird

[ljnelson]

ljnelson  wrote:

I'll put together another pull request that incorporates my reply to ochaloup .  I think it is easier to simply write it than to explain it.  :-)

See Comparing jbosstm:master...ljnelson:remove-jndi · jbosstm/narayana · GitHub .  Untested.

Best,

Laird

[tomjenkinson]

Thanks for the contribution so far and it looks great, but as Ondra quite rightly pointed out to me I think in the appserver we do need to obtain the reference by JNDI because they wrap our own TransactionManager instance with their own. Is it possible to provide the extension points you would need but still keep the default way still to use JNDI:

initialContext.lookup(jtaPropertyManager.getJTAEnvironmentBean().getTransactionManagerJNDIContext() and initialContext.lookup(jtaPropertyManager.getJTAEnvironmentBean().getTransactionSynchronizationRegistryJNDIContext

[ljnelson]

tomjenkinson  wrote:

Thanks for the contribution so far and it looks great, but as Ondra quite rightly pointed out to me I think in the appserver we do need to obtain the reference by JNDI because they wrap our own TransactionManager instance with their own. Is it possible to provide the extension points you would need but still keep the default way still to use JNDI:

initialContext.lookup(jtaPropertyManager.getJTAEnvironmentBean().getTransactionManagerJNDIContext() and initialContext.lookup(jtaPropertyManager.getJTAEnvironmentBean().getTransactionSynchronizationRegistryJNDIContext

Well, remember that TransactionManager in my pull request is just a CDI bean, so yes.  That means other CDI extensions can veto it and install their own.  So you're saying that the default CDI bean here should produce a TransactionManager by looking it up from JNDI?  I'll update my pull request to reflect this.

[ljnelson]

I've updated my fork here: Comparing jbosstm:master...ljnelson:remove-jndi · jbosstm/narayana · GitHub

At a high level, I first check to see if there are any TransactionManager and TransactionSynchronizationRegistry CDI beans available.  If there aren't any, I register some that use JNDI.  To use JNDI, I check to see if there are any beans registered for InitialContext.  If there aren't any, then I add one.

The net effect of all of this is that if the end user supplies her own TransactionManager bean, for example, then the existing TransactionExtension will simply use it, and if she does not, then a bean backed by JNDI will be used instead.

Best,

Laird

[tomjenkinson]

Hi Laird,

That looks good thanks, feel free to raise the PR now and let's see if CI spots any problems with it.

Thanks again,

Tom

[tomjenkinson]

I have noticed that the no-arg public constructor was removed, we would need that back to not break compatability in a minor release.