Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dom4j concurrency problem #40

Closed
jbennett2091 opened this issue Feb 13, 2018 · 0 comments
Closed

Dom4j concurrency problem #40

jbennett2091 opened this issue Feb 13, 2018 · 0 comments
Assignees
Labels
Milestone

Comments

@jbennett2091
Copy link
Contributor

This is related to Issue #15

The concurrency problem that has been previously raised is caused by misuse of SynchronizedMap. As noted in SynchronizedMap's javadoc, a synchronized-block must be placed around things that iterate the map (c.f. https://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#synchronizedMap-java.util.Map-). The QNameCache.getQNames() method fails to do this resulting in the possibility of ConcurrentModificationExceptions if getQNames() is iterating in thread#1 while thread#2 is mutating the map.

The use of a ConcurrentHashMap (as proposed as an enhancement in #15) would address this, but is complicated by a desire to preserve the WeakReferences within the map. That is, a ConcurrentWeakHashMap would solve it, but that is a heavy lift.

Instead, I would propose we simply follow the recommendation of SynchronizedMap. Namely:

    public List<QName> getQNames() {
        List<QName> answer = new ArrayList<QName>();
        synchronized(noNamespaceCache) {
            answer.addAll(noNamespaceCache.values());
        }

        synchronized(namespaceCache) {
            for (Map<String, QName> map : namespaceCache.values()) {
                answer.addAll(map.values());
            }
        }

        return answer;
    }

This will eliminate the bug and make Dom4j thread-safe. Implementing #15 in the future would additionally make it performant.

jbennett2091 added a commit to jbennett2091/dom4j that referenced this issue Feb 13, 2018
Solution proposed by Issue dom4j#40
@FilipJirsak FilipJirsak self-assigned this Feb 13, 2018
@FilipJirsak FilipJirsak added this to the 2.1.1 milestone Feb 13, 2018
FilipJirsak pushed a commit that referenced this issue Jul 1, 2018
Solution proposed by Issue #40
FilipJirsak pushed a commit to FilipJirsak/dom4j that referenced this issue Mar 12, 2020
Solution proposed by Issue dom4j#40
FilipJirsak pushed a commit that referenced this issue Apr 11, 2020
Solution proposed by Issue #40

(cherry picked from commit b3d9226)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants