Bug 50648

Summary: RpcChannel improvements
Product: Tomcat 7 Reporter: Olivier Costet <ocostet>
Component: ClusterAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: enhancement CC: ocostet
Priority: P2 Keywords: PatchAvailable
Version: 7.0.6   
Target Milestone: ---   
Hardware: PC   
OS: All   
Attachments: svn diff

Description Olivier Costet 2011-01-25 09:38:16 UTC
Created attachment 26545 [details]
svn diff

This enhancement proposal addresses three separate issues with the RpcChannel tribes component.

1. There is a minor bug in a catch(InterruptedException) clause, in which a call to Thread#interrupted() is made, as opposed to Thread#interrupt(), as it should be.

2. The RpcChannel API user registers an RpcCallback to be notified of communication events. However, there is no callback method for the case where the sending of a response message fails. This proposal includes the addition of a corresponding callback for such cases. To ensure backwards-compatibility, the additional callback method has been declared in a sub-interface of RpcCallback, named RpcCallback2 for the time being.

3. Since the tribes classes are loaded by the tomcat loader, if the messages sent through tribes are instances of classes defined in the webapp, attemps to deserialize them will fail. AbstractReplicatedMap includes a mechanism for the API user to specify class loaders to be used when deserializing map entries. This enhancement proposal includes a similar mechanism for the RpcChannel.

Please find the proposed modifications attached.
Comment 1 Mark Thomas 2011-01-26 11:53:45 UTC
(In reply to comment #0)
> This enhancement proposal addresses three separate issues with the RpcChannel
> tribes component.

Separate issues should have separate Bugzilla entries and separate patches. Trying to manage multiple issues with a single Bugzilla entry quickly gets to be a pain.

> 1. There is a minor bug in a catch(InterruptedException) clause, in which a
> call to Thread#interrupted() is made, as opposed to Thread#interrupt(), as it
> should be.

Fixed in 7.0.x and will be included in 7.0.7 onwards.


> 2. The RpcChannel API user registers an RpcCallback to be notified of
> communication events. However, there is no callback method for the case where
> the sending of a response message fails. This proposal includes the addition of
> a corresponding callback for such cases. To ensure backwards-compatibility, the
> additional callback method has been declared in a sub-interface of RpcCallback,
> named RpcCallback2 for the time being.

Please open a new issue for this enhancement.
The API isn't fixed for 7.0.x so just add the method to the RpcCallback interface. 
Patches should be the minimum to address the issue and should use the same style as the existing code.

> 3. Since the tribes classes are loaded by the tomcat loader, if the messages
> sent through tribes are instances of classes defined in the webapp, attemps to
> deserialize them will fail. AbstractReplicatedMap includes a mechanism for the
> API user to specify class loaders to be used when deserializing map entries.
> This enhancement proposal includes a similar mechanism for the RpcChannel.

Please open a new issue for this enhancement request.
Patches should be the minimum to address the issue and should use the same style as the existing code.
Comment 2 Filip Hanik 2011-01-26 12:39:40 UTC
2. 
What type of exceptions have you seen here?
Reply failed callback, is a strange callback, since the receiver, not the sender would receive that callback. That would make for some confusing programming logic. The sender will simply timeout, and not get the reply, and has to act accordingly. But I'm not sure the receiver needs to be notified.

3.
The external loaders IMHO don't belong here.
For the applications that wish to provide custom class loading, I would simply send messages using the ByteMessage class. That way you have full control over what is happening.
Comment 3 Olivier Costet 2011-01-27 06:17:20 UTC
@Mark
Sorry about that. I'll file seperate entries and will try to match the style.


(In reply to comment #2)
> 2. 
> What type of exceptions have you seen here?
> Reply failed callback, is a strange callback, since the receiver, not the
> sender would receive that callback. That would make for some confusing
> programming logic. The sender will simply timeout, and not get the reply, and
> has to act accordingly. But I'm not sure the receiver needs to be notified.

I haven't /seen/ any exceptions, but I would assume any exception that can be thrown by a normal Channel#send could come up here, since a Channel#send is what happens.
The RpcCallback interface contains both callbacks that are invoked on the sender (leftOver) and receiver (replyRequest) side. If that isn't confusing, I don't see why adding one would be. 
The specific case where I felt the need for such a callback was one where I was shuttling instances across nodes -- by "shuttling" I mean removing them from one and putting them on the other. I would receive a request through the RpcChannel for an instance, look it up, unregister it locally, pack it in the reply, send it. If the sending failed, I would want to re-register it locally, so as not to lose data.
Whatever you may think of that usage, the fact remains that there's a discrepancy between requests on an RpcChannel, where the sender is notified of communication errors, and replies on the RpcChannel, where he (the sender of the reply) isn't. Even though in both cases it's the same Channel#send that is performed.

 
> 3.
> The external loaders IMHO don't belong here.
> For the applications that wish to provide custom class loading, I would simply
> send messages using the ByteMessage class. That way you have full control over
> what is happening.

It's true, you could use ByteMessages. Although as an API user, your code would become more clunky, and you'd lose the ability to quickly look up the message's class.
Wouldn't it be nice to have, though? It makes things easier, cleaner and doesn't add any significant overhead. Also, as things stand, the tribes API is somewhat misleading in that it offers to send Serializable messages (methods like Channel#send take Serializable arguments), when in practice all your app's classes are excluded, no matter how Serializable they may be.
Comment 4 Olivier Costet 2011-01-27 08:50:53 UTC
Filed separe enhancement requests:
https://issues.apache.org/bugzilla/show_bug.cgi?id=50667 (replyFailed)
https://issues.apache.org/bugzilla/show_bug.cgi?id=50670 (class loaders)