Hi leehui.
I agree your patch and I also experienced same error!
But if NetworkManager.hash() is not thread safe, I think "synchronized" block should be used more widely.
Like this:
private static byte[] hash(final String expression) {
byte[] digArray;
if (expression == null) {
throw new IllegalArgumentException("Invalid null expression");
}
if (digest == null) {
synchronized( this ) {
if( digest == null ) {
try {
digest = MessageDigest.getInstance("SHA1");
} catch (NoSuchAlgorithmException ex) {
LOG.log(Level.WARNING, ex.getLocalizedMessage());
}
}
}
}
synchronized (digest) {
digest.reset();
try {
digArray = digest.digest(expression.getBytes("UTF-8"));
} catch (UnsupportedEncodingException impossible) {
LOG.log(Level.WARNING, "digestEncoding unsupported:"
+ impossible.getLocalizedMessage() +
":returning digest with default encoding");
digArray = digest.digest(expression.getBytes());
}
}
return digArray;
}
As you know, actually my edit from your patch is trivial. :-)
Thanks.
--
Bongjae Chang
----- Original Message -----
From: "leehui" <leehui70_at_gmail.com>
To: "dev" <dev_at_shoal.dev.java.net>
Sent: Monday, June 16, 2008 10:13 AM
Subject: [Shoal-Dev] find the cause of sendMessage bug
> Hi,
>
> First of all, thanks for shreedhar's advice.
>
> I check the latest code, and try to fix the sendMessage bug.
>
> The symptom is sending messages to a member may fail sometimes though that member is still alive. I debug the shoal and find the causes of the bug.
>
> In the NetworkManager class, there is a static method named hash. The following codes are original.
>
> private static byte[] hash(final String expression) {
> byte[] digArray;
> if (expression == null) {
> throw new IllegalArgumentException("Invalid null expression");
> }
> if (digest == null) {
> try {
> digest = MessageDigest.getInstance("SHA1");
> } catch (NoSuchAlgorithmException ex) {
> LOG.log(Level.WARNING, ex.getLocalizedMessage());
> }
> }
> digest.reset();
> try {
> digArray = digest.digest(expression.getBytes("UTF-8"));
> } catch (UnsupportedEncodingException impossible) {
> LOG.log(Level.WARNING, "digestEncoding unsupported:"
> + impossible.getLocalizedMessage() +
> ":returning digest with default encoding");
> digArray = digest.digest(expression.getBytes());
> }
> return digArray;
> }
>
> You must notice that MessageDigest is not thread-safe.
> When we use sendMessage method to send a msg to a destination member, the sendMessage method first uses the above hash method to get peer group id, then verifies the peer group id is in its view. After that, the sendMessage method sends msg data to the destination member. Therefore, when multiple threads invoke sendMessage method, the above hash method will be invoked by multiple threads. As MessageDigest is not thread-safe, the peer group id returned by the hash method may be wrong, and it may not contained in current member's view. At that time, the console prints following info.
>
> java.lang.ArrayIndexOutOfBoundsException
> at java.lang.System.arraycopy(Native Method)
> at sun.security.provider.DigestBase.engineUpdate(DigestBase.java:102)
> at sun.security.provider.SHA.implDigest(SHA.java:94)
> at sun.security.provider.DigestBase.engineDigest(DigestBase.java:161)
> at sun.security.provider.DigestBase.engineDigest(DigestBase.java:140)
> at java.security.MessageDigest$Delegate.engineDigest(MessageDigest.java:531)
> at java.security.MessageDigest.digest(MessageDigest.java:309)
> at java.security.MessageDigest.digest(MessageDigest.java:355)
> at com.sun.enterprise.jxtamgmt.NetworkManager.hash(NetworkManager.java:222)
> at com.sun.enterprise.jxtamgmt.NetworkManager.getPeerGroupID(NetworkManager.java:272)
> at com.sun.enterprise.jxtamgmt.NetworkManager.getInfraPeerGroupID(NetworkManager.java:362)
> at com.sun.enterprise.jxtamgmt.NetworkManager.getPeerID(NetworkManager.java:261)
> at com.sun.enterprise.jxtamgmt.ClusterManager.getID(ClusterManager.java:662)
> at com.sun.enterprise.ee.cms.impl.jxta.GroupCommunicationProviderImpl.sendMessage(GroupCommunicationProviderImpl.java:226)
> at com.sun.enterprise.ee.cms.impl.jxta.GroupHandleImpl.sendMessage(GroupHandleImpl.java:131)
> at MultiThreadMessageSender$1.run(MultiThreadMessageSender.java:52)
> at java.lang.Thread.run(Thread.java:595)
> 2008-6-12 11:20:21 com.sun.enterprise.ee.cms.impl.jxta.GroupHandleImpl sendMessage
> ¾¯¸æ: GroupHandleImpl.sendMessage : Could not send message : Member B is not in the View anymore. Hence not performing sendMessage operation
>
> So, I modify the NetworkManager codes as following.
>
> private static byte[] hash(final String expression) {
> byte[] digArray;
> if (expression == null) {
> throw new IllegalArgumentException("Invalid null expression");
> }
> if (digest == null) {
> try {
> digest = MessageDigest.getInstance("SHA1");
> } catch (NoSuchAlgorithmException ex) {
> LOG.log(Level.WARNING, ex.getLocalizedMessage());
> }
> }
> synchronized (digest) {
> digest.reset();
> try {
> digArray = digest.digest(expression.getBytes("UTF-8"));
> } catch (UnsupportedEncodingException impossible) {
> LOG.log(Level.WARNING, "digestEncoding unsupported:"
> + impossible.getLocalizedMessage() +
> ":returning digest with default encoding");
> digArray = digest.digest(expression.getBytes());
> }
> }
> return digArray;
> }
>
> As your see, I just add synchronized statement to surround the digest operation. I have tested the code. It works well.
>
> Of course, if we add some codes to caching the ID, we don't need to calculate ID for each sending message. It's a better choice.
>
> Best regards!
>
> --------------
> leehui
> 2008-06-16
>