Concurrent modification exception in HttpResponseCache.uriToKey about httpresponsecache HOT 11 CLOSED

Hrmm... this does appear to be a bug in Android. The code in HttpResponseCache is thread safe. Here's the uriToKey implementation:

    private String uriToKey(URI uri) {
        try {
            MessageDigest messageDigest = MessageDigest.getInstance("MD5");
            byte[] md5bytes = messageDigest.digest(Strings.getBytes(uri.toString(),Charsets.UTF_8));
            return Strings.bytesToHexString(md5bytes, false);
        } catch (NoSuchAlgorithmException e) {
            throw new AssertionError(e);
        }
    }

The MessageDigest is not reused between threads.

The exception is actually triggered inside the MessageDigest.getInstance() implementation. Which is core Android. I checked the Android issue tracker, and I cannot find any other similar bug reports.

What version of Android are you using? Can you please report the issue at https://code.google.com/p/android/issues/entry ?

from httpresponsecache.

Some more searching indicates that there was a bug in the IBM JDK: https://www-01.ibm.com/support/docview.wss?uid=swg1IV02334 So I'm willing to bet this bug was in Apache Harmony when Android forked it. I have no idea if this issue was fixed in a later Android version than you're using - I'm quite interested to learn what you're using. And please link me to that bug report when you create it.

We can probably work around the issue by wrapping the call to MessageDigest.getInstance("MD5") in a try/catch block and wrap that in a do/while block... I'm not thrilled about it, but it should work. Can I provide you with a jar to test, seeing as how I cannot reproduce the problem?

from httpresponsecache.

Hi Craig,

I tested my application using the HttpResponseCache coming with the system
so the same symptom. According to the documentation of MessageDigest it is
not thread safe so overall even if the HttpResponseCache is thread safe in
it self I assume in theory it might fail if another part of an app is using
MessageDigest.

I am currently trying to pinpoint down exactly what other parts in my app
that actually triggers the error so let me come back when I have some more
information.

I will also write a ticket as you suggested on Google, but it would be
really nice if I could narrow it down and give them an easy way to
reproduce it.

Regarding your solution I am quite sure it will work and no problems if you
want me to test your fix. However, I was also thinking that it maybe is
possible to fix it in a more clean way, I have not tried it out yet but my
idea is to actually create a clone of the MD5 message digest and stored it
as a static instance in the HttpResponseCache (I mean the generated file
names only have to be unique within the app).

So in pseudo code something like this.

1. When HttpResponeCache is initialized.

static MessageDigest localMd5Digester;

MessageDiggest digester = MessageDigest.getInstance("MD5");
localMd5Digester = digester.clone();

In get file name mapping one could use the localMd5Digester instead of the
original one. I am not sure how solid it is as I assume that the clone is a
shallow copy if I am correct.

Best regards/
Lars

2012/9/26 Craig [email protected]

Some more searching indicates that there was a bug in the IBM JDK:
https://www-01.ibm.com/support/docview.wss?uid=swg1IV02334 So I'm willing
to bet this bug was in Apache Harmony when Android forked it. I have no
idea if this issue was fixed in a later Android version than you're using -
I'm quite interested to learn what you're using. And please link me to that
bug report when you create it.

We can probably work around the issue by wrapping the call to
MessageDigest.getInstance("MD5") in a try/catch block and wrap that in a
do/while block... I'm not thrilled about it, but it should work. Can I
provide you with a jar to test, seeing as how I cannot reproduce the
problem?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/22#issuecomment-8892855.

from httpresponsecache.

Hi Craig,

I have created the following google issue:
https://code.google.com/p/android/issues/detail?id=37937

I was able to isolate the cause as I described in the above issue. The problem in the HttpResponseCache is that it uses the non thread safe MessageDigest core class and if any other part of the application is using MessageDigest.getInstance in another thread the problem can happen randomly.

In my case I was a little bit sloppy and had forgotten to add a serialVersionUID to a custom object that I serialized to the sdcard for cashing and if its missing the core will try to generate one using MessageDigest.

I took down your code locally and created a clone of MessageDigest and I could not see any more crashes in the HttpUrlResponseCache after that. Don't know if its the best solution but according to the Google documentation for the MessageDigest this is called a partial digester. (I have copied the code pieces in to the 37937 issue ticket as a suggestion how to solve it.

Best regards/
Lars

from httpresponsecache.

I've committed a workaround using your clone approach: a93c92d

Thanks for your help - hopefully the issue in Android will get resolved.

from httpresponsecache.

Hi Craig,

I checked your commit and you need to adjust it a little bit to get it to
work. The clone of the digest has to be done in the constructor to minimize
the chance for a concurrent modification exception, i.e. the commited fix
is no different than the original.

So it need to be like this, see step 1 and 2 below.

Best regards/
Lars

private MessageDigest partialMessageDigester;

1. Perform the clone in the constructor of the cache, this is
   hopefully done early in the app.

   public HttpResponseCache(File directory, long maxSize) throws IOException {
   cache = DiskLruCache.open(directory, VERSION, ENTRY_COUNT, maxSize);
   try {
   //1. Fetch the digester and store it locally, this is the
   fallback if clone is not supported
   partialMessageDigester = MessageDigest.getInstance("MD5");

      //2. Clone the digester and assign to the partial message digester
       partialMessageDigester = (MessageDigest)
   

   partialMessageDigester.clone();
   } catch (NoSuchAlgorithmException e) {
   throw new AssertionError(e);
   } catch (CloneNotSupportedException e) {
   e.printStackTrace();
   }
   }

2. Now this method will use the cloned object.

   private String uriToKey(URI uri) {
   // Use the partial digester
   byte[] md5bytes =
   partialMessageDigester.digest(Strings.getBytes(uri.toString(),
   Charsets.UTF_8));
   return Strings.bytesToHexString(md5bytes, false);
   }

2012/9/28 Craig [email protected]

I've committed a workaround using your clone approach: a93c92dhttps://github.com/candrews/HttpResponseCache/commit/a93c92d1d8b87778ac7bb2fa2fe2759e7ca06219

Thanks for your help - hopefully the issue in Android will get resolved.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/22#issuecomment-8986057.

from httpresponsecache.

You can't reuse the partialMessageDigester as you do in (2).

So I think what needs to be done is:
In the constructor:
while(partialMessageDigester == null){
try{
partialMessageDigester = MessageDigest.getInstance("MD5");
}catch{ConcurrentModificationException e)
//ignore
}
}

Then, when it's used:
byte[] md5bytes = partialMessageDigester.clone().digest(Strings.getBytes(uri.toString(), Charsets.UTF_8));

Thoughts?

This is a really pain.

from httpresponsecache.

Hi Craig,

Ok sorry missed that it is not possible to reuse it :).

Yes I agree its really a pain to find a solid solution for this one.

Regarding your suggestion, thats a tricky implementation and I was thinking
of the following two drawbacks:

1. Clone is probably not thread-safe so we might not improve here
2. Is clone correctly supported on all android platforms?

So I was thinking of two other options as well:

1. Revert the fix back to the original one and make a nice documentation in
   the release notes of the issue and highlight that one should be careful
   using the MessageDigest.getInstance("MD5") in combination in other threads.
   As Java serialization will try to generate hashes automatically using the
   MessageDigest.getInstance("MD5") if no serialVersionUID is set make sure to
   have them set on the classes (This is anyway good practice and should
   always be done) if the app wants to perform serialization concurrently.
2. I Googled quickly and found two implementation of MD5 that could be used
   instead of the one from core.

http://ostermiller.org/utils/src/MD5.java.html

http://www.twmacinta.com/myjava/fast_md5.php

I think (1) is the easiest one and we could see if Google has some good
suggestions. Number (2) should work as well but is more job.

What do you think?

Best regards/
Lars

2012/9/28 Craig [email protected]

You can't reuse the partialMessageDigester as you do in (2).

So I think what needs to be done is:
In the constructor:
while(partialMessageDigester == null){
try{

partialMessageDigester = MessageDigest.getInstance("MD5");
}catch{ConcurrentModificationException e)
//ignore
}
}

Then, when it's used:
byte[] md5bytes =
partialMessageDigester.clone().digest(Strings.getBytes(uri.toString(),
Charsets.UTF_8));

Thoughts?

This is a really pain.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/22#issuecomment-8987960.

from httpresponsecache.

With this level of breakage, I agree with you that simply not using the build in MD5 implementation is the best idea.

http://org.rodage.com/pub/java/security/MD5.java is in the public domain, so I'll try using it.

I don't think adding documentation explaining that developers should declare serialVersionUIDs is a good idea; that problem is not related to HttpResponseCache. If you have a test case showing that problem, I bet reporting that to Google would get them going on fixing this MessageDigest problem faster.

I can't believe others have fun into bug - it seems fairly fundamental.

from httpresponsecache.

Hi Craig,

yes you are right and its a good solution :)

you should have a 1000% credit for this project, its one of my top
favorites next to the ActionBarSherlock and the ViewPagerIndicator
projects.

Best regards/
Lars

2012/9/28 Craig [email protected]

With this level of breakage, I agree with you that simply not using the
build in MD5 implementation is the best idea.

http://org.rodage.com/pub/java/security/MD5.java is in the public domain,
so I'll try using it.

I don't think adding documentation explaining that developers should
declare serialVersionUIDs is a good idea; that problem is not related to
HttpResponseCache. If you have a test case showing that problem, I bet
reporting that to Google would get them going on fixing this MessageDigest
problem faster.

I can't believe others have fun into bug - it seems fairly fundamental.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/22#issuecomment-8989587.

from httpresponsecache.

I think we may have finally gotten to a good state: eaeaeb2

Let me know if there are any other improvement ideas or suggestions

from httpresponsecache.