[galder.zamarreno]

After finishing implementing jbas-4455 for JRMP, I have just started with unified invokers and I have realised that UnifiedInvokerProxy/UnifiedInvokerProxyHA maintain a
instance reference to the InvokerLocator/Client instances to a specific invocation is using. That is, when a new target is chosen, the unified proxy
updates its Client and corresponding InvokerLocator instances (if they've changed), and uses the Cient instance variable to make the call. As far
as I can see there's no synchronisation in the usage/assignment of Client or InvokerLocator. Proxies are meant to be thread safe which means that
multiple client app threads could be using the same proxy without any problems. I can see all this going wrong the moment someone uses client
proxies this way in a clustered environment with round robin load balance policy. We've got no guarantee that round robin will work correctly in these situations.

JRMP based invokers never had this problem because they never stored the target server invoker used as instance variable. Why should Unified
invoker proxies do so? IMO, it shouldn't. In a clustered environment, such instance variable could be constantly changing, i.e. round robin.

Secondly, looking at FirstAvailable, electedTarget access/update is not synchronised, however, the chances of having some issues are very small. First, the proxy needs to be shared by various threads before the electedTarget has been set, but as electedTarget already elected randomly, the secondary effects of this lack of safety are none.

[brian.stansberry]

Please create JIRAs for these. If you can fix them, even better. :-)

I've only taken a one minute glance at the relevant UnifiedInvokerHA code. Would some synchronization in the UnifiedInvokerHAProxy.getClient code solve the problem? I believe UnifiedInvokerHAProxy.invoke does not use the client instance field, but rather a local variable.

A map of Clients keyed by target is also a possibility, although that's a more complex solution.

[galder.zamarreno]

Please create JIRAs for these. If you can fix them, even better. :-)

I'll create them and try to fix them too :)

I've only taken a one minute glance at the relevant UnifiedInvokerHA code. Would some synchronization in the UnifiedInvokerHAProxy.getClient code solve the problem? I believe UnifiedInvokerHAProxy.invoke does not use the client instance field, but rather a local variable.

A map of Clients keyed by target is also a possibility, although that's a more complex solution.

UnifiedInvokerHAProxy.getClient() returns the client instance variable. This is set in init(InvokerLocator) which gets called once only for UnifiedInvokerProxy. However in UnifiedInvokerHAProxy, this method is called whenever the cached locator is different to the one returned by the loca balance policy. So, in a round robin scenario, the client instance variable (which is accessed via getClient) would be changing constantly.

Synchronising getClient() could resolve the issue. Need to look more in depth. Seeing this, it is my intention to create a UT that tests thread safety as part of jbas-4455. It'd probably comment it when committing jbas-4455, so that it remains independent, and then resolve it when I fix that jira.

Another simple solution would be not to used the cached Client (like in jrmp). However, this could mean calling connect every time the Client is used. I need to look at Client in case this could be avoided by some isConnected() method.

[brian.stansberry]

The main thing is the work of UnifiedInvokerHAProxy.getClient(InvokerLocator) needs to be atomic; i.e. the client that gets configured in the init(InvokerLocator) call is the one that gets returned. Storing the result in a single instance field is a reasonable optimization, since it shouldn't change often.

[galder.zamarreno]

http://jira.jboss.com/jira/browse/JBAS-4815
http://jira.jboss.com/jira/browse/JBAS-4816

[galder.zamarreno]

I had a prototype of this solution for a while, but I had encountered some issues while attempting to finalise it. These issues are (except for EJB3) now resolved and while looking back at the prototype, I came across this:

At the beginning of the invocation, I do a get on the tx failover map passing the current tpc. If it returns not null, I add it to the invocation transient payload so that the load balance policy can choose the right target. Also, after the invocation has returned, I get the transaction sticky target from the transient map and put it with the tpc in the tx failover map. So far so good.

At the moment, before these two sections, I check whether the load balance policy is instanceof TransactionSticky. This class does the work of checking whether a chosen target is in the invocation transient payload and if it isn't, delegates to a standard lbp). There're 4 matching versions (FA, R....etc) of TransactionSticky, each containing a delegate for the standard type. These matching lbps extend TransactionSticky.

There's a problem though with just doing instanceof TransactionSticky. Pre transaction sticky clients would get a nice CNFE. I can see two alternatives:

1.- do not check instanceof TransactionSticky and do the lookup in the tx failover map and put the result as chosen target in the invocation transient map regardless. For non sticky versions, it would null would be returned from the map lookup and null would be put in the inv transient map. This would mean that all transactional invocations would have 3 added map operations added to the cost of the invocation. Not good and makes the code less readable.

2.- add a transient boolean instance variable to each proxy with the value of T.getCurrentT().getCCL().loadClass(TransactionSticky) being successfull AND lbp instanceof TransactionSticky returning true. I could then use this parameter in the proxy to avoid unnecessary map get/puts. This solution would safe 3 map operations per transactional invocation for non tx sticky situations.

Both this solutions avoid possible issues with pre transaction sticky clients that won't have those classes. My personal preference is with 2, although I'm not 100% sure about the cost of a loadClass() vs cost of a map operation. I do imagine though that if clients cache the proxy and reuse it, the having the transient boolean option would perform much better.

[brian.stansberry]

I don't think #2 does much for you. Even if the LBP used by the proxy isn't "TransactionSticky" it's quite likely the class will be on the classpath, as it's packaged in the same jar as the other LBP impls.

Anyway, -1 to it because you are hard-coding things so it only works if *your* TransactionSticky is on the classpath.

Keep it simple; don't stress too much about a couple map operations. Walk a debugger through a complete method invocation on an EJB; the amount of stuff it goes through is amazing. :)

[galder.zamarreno]

Ok, ic. Keeping the proxy implementations independent of any specific LBP has indeed some benefits.

I'll keep it simple :)

[galder.zamarreno]

I have also seen that you have implemented the new load balance policies that didn't leak Invocation.

As the new lbps use the Invocation parameter to check whether it contains a transient sticky target i'll be creating them in the org.jboss.ha.framework.interfaces package.

[brian.stansberry]

Yes. In the AS cluster module, as we can't leak the legacy Invocation class to the ha-client project.

[galder.zamarreno]

Back once again (like the renegade master :))

trunk and 4.2.x differ slightly when it comes to org.jboss.invocation.MarshalledInvocation class.

in 4.2.x, MarshalledInvocation(Invocation invocation) constructor does:

...
this.transient_payload = invocation.transient_payload;
this.invocationContext = invocation.invocationContext;
...

whereas in trunk, the transient payload is not copied over to the MarshalledInvocation.

The discrepancy comes from:

http://viewvc.jboss.org/cgi-bin/viewvc.cgi/jbossas/branches/Branch_4_2/server/src/main/org/jboss/invocation/MarshalledInvocation.java?r1=38132&r2=41001

as part of:

"JBAS-2436 - Commiting Pluggable Serialization into JBAS 4.0" (http://jira.jboss.com/jira/browse/JBAS-2436)

Whether the transient payload is copied over to the MarshalledInvocation is not very important because eventually, writeExternal won't write it. However, why would you copy over a transient value over to an invocation that will be marshalled and which will not include transient values??

This discrepancy is making the tests that run in trunk not work in 4.2.x. I can workaround this but I don't think this type of discrepancies are good.

Thoughts? Worth a JIRA?

[galder.zamarreno]

In fact, JBAS-2436 is about making serialization of objects between proxy and invoker pluggable. This was not done in trunk or if it was done, was reverted. However, I think this is a different matter to my last comment.

[brian.stansberry]

JIRA -- that you immediately fix :). The values should be copied. Without them the Invocation API contract cannot be properly met (e.g. Invocation.getInvocationContext(), Invocation.getTransientValue()).

Maybe those methods are never called (except, I guess, now they are called in your tests ;)) but they are part of the API and properly supporting the API is worth the cost of a couple of assignment operations.

[galder.zamarreno]

JIRA on its way. I'll fix it asap in trunk and will make sure I fix the invoker ha tests as part of that, otherwise they'll fail.

Some of my post execution assertions were checking values in the transient payload which is where my tests failed when porting them to 4.2.x.