#854 ✓ resolved
Miklos Gyorfi

Unsychnronized internal cache in Binding

Reported by Miklos Gyorfi | May 19th, 2011 @ 02:09 PM | in 1.3

Framework version: 1.2.1
Platform you're using: Windows/Linux JDK 1.6

Reproduction steps: Hardly to reproduce, because it is a thread safety issue, and the result turns out more later.

Details:

In the Binder there is a cache map storing BeanWrapper instances


    static Map<Class<?>, BeanWrapper> beanwrappers = new HashMap<Class<?>, BeanWrapper>();

     static BeanWrapper getBeanWrapper(Class<?> clazz) {
        if (!beanwrappers.containsKey(clazz)) {
            BeanWrapper beanwrapper = BeanWrapper.forClass(clazz);
            beanwrappers.put(clazz, beanwrapper);
        }
        return beanwrappers.get(clazz);
     }

Binding is called from requests' threads, so it should be thread safe. This code is not safe, as it uses unsynchronized shared object (the Map). However using synchronized block is too poor solution.

My solution is the usage of the ConcurrentHashMap, and put the cache into the BeanWrapper class. As BeanWrapper and Property instances are shared as well, they should be strictly immutable (it is possible).

I can contribute this new code.

Comments and changes to this ticket

  • Morten Kjetland
  • Miklos Gyorfi

    Miklos Gyorfi June 8th, 2011 @ 02:04 PM

    Ok, due to the rebasing problem (I'm not a git-expert, that's true), I've made a new pull request:

    https://github.com/playframework/play/pull/259

    Some Q&A for Morten Kjetland:

    Q: It was difficult to see your actual changes due to to much changed code - can you please only change the code/comment/docs needed ? Don't auto change imports, add linebreaks etc where not needed.
    A: Ok, i've cleaned the code

    Q: Your caching solution will not work when Java objects are recompiled since you will have the old beanwrapper cached.
    A: Yes, that's true. But I've worked out the threading issue, and this caching techinque have been existed in the original version as well, not in the BeanWrapper, but in the Binding class. If something is recompiled, a new class (even if with the same name, it's classloader have been changed, that's why it is not the same class) would be created, and the cache binds the wrapper to the class not to the name. Same way as the old buggy did. The old class stucked into the memory, but I've thought it was not a problem, as the old cache works the same way. Am I right?

  • Play Duck

    Play Duck June 8th, 2011 @ 08:46 PM

    (from [84b47222edd07e4f0e9f6a60f3adad404499b84e]) [#854] Fixes unsynchronized internal cache in Binding Solution: new caching method, and immutable BeanWrapper and Property info.
    https://github.com/playframework/play/commit/84b47222edd07e4f0e9f6a...

  • Morten Kjetland

    Morten Kjetland June 8th, 2011 @ 08:48 PM

    • Milestone set to 1.3
    • State changed from “new” to “resolved”
    • Assigned user set to “Morten Kjetland”
    • Milestone order changed from “581” to “0”

    Thanks a lot! It's merged. I guess you are right about the recompiling stuff

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Play framework

Play makes it easier to build Web applications with Java. It is a clean alternative to bloated Enterprise Java stacks. It focuses on developer productivity and targets RESTful architectures. Learn more on the http://www.playframework.org website.

Source code is hosted on github

Check out our repository at http://github.com/playframework/play

Contributing, creating a patch

Please read the contributor guide

Reporting Security Vulnerabilities

Since all bug reports are public, please report any security vulnerability directly to guillaume dot bort at gmail dot com.

Creating a bug report

Bug reports are incredibly helpful, so take time to report bugs and request features in our ticket tracker. We’re always grateful for patches to Play’s code. Indeed, bug reports with attached patches will get fixed far quickly than those without any.

Please include as much relevant information as possible including the exact framework version you're using and a code snippet that reproduces the problem.

Don't have too much expectations. Unless the bug is really a serious "everything is broken" thing, you're creating a ticket to start a discussion. Having a patch (or a branch on Github we can pull from) is better, but then again we'll only pull high quality branches that make sense to be in the core of Play.

Shared Ticket Bins

Referenced by

Pages