[
https://issues.apache.org/jira/browse/PDFBOX-5056?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17255895#comment-17255895
]
Mike Kaplinskiy commented on PDFBOX-5056:
-----------------------------------------
If you just mark contains synchronized that will work. The “bad” code is still
broken, assuming there are multiple threads involved (I’m not sure exactly how
that can happen - I don’t know pdfbox well enough).
The solution you committed seems correct. I think sonar just isn’t smart enough
to figure out the set is immutable after assignment. There’s several other ways
to fix the problem that might also please sonar. Here’s some ideas:
{code}
public boolean contains(String name)
{
return inverted.contains(name);
}
{code}
Because {{inverted}} has keys that are the values in {{codeToName}}. I’m a
little suspect of synchronization in the entire class (Encoding) generally -
like why puts and other gets aren’t synchronized - but that’s a separate
problem. I don’t have enough context to know whether instances of encoding are
mutable shared between threads.
Other solutions:
{code}
private Set<String> names;
public synchronized boolean contains(String name)
{
// we have to wait until all add() calls are done before building the
name cache
// otherwise /Differences won't be accounted for
if (names == null)
{
names = new HashSet<String>(codeToName.values());
}
return names.contains(name);
}
{code}
That is - don’t do double checked locking. Realistically this is testing an
element in a set - the critical section is tiny cpu time wise and the lock is
unlikely to hurt.
> Double checked locking done wrong in several places
> ---------------------------------------------------
>
> Key: PDFBOX-5056
> URL: https://issues.apache.org/jira/browse/PDFBOX-5056
> Project: PDFBox
> Issue Type: Bug
> Affects Versions: 2.0.22
> Reporter: Mike Kaplinskiy
> Priority: Major
>
> There's several places inside pdfbox where double-checked locking is done
> wrong. Specifically, double checked locking is the pattern of:
> {code:java}
> private volatile boolean doneInit = false;
> if (!doneInit) {
> synchronized (this) {
> if (!doneInit) {
> ... do init
> doneInit = true;
> }
> }
> }{code}
> Common issues are - lack of {{volatile}} or the volatile set not being last.
> Here are the cases I found so far:
> *
> [https://github.com/apache/pdfbox/blob/e409f25009702be8889ce586b8f6dd7274201f0a/pdfbox/src/main/java/org/apache/pdfbox/pdmodel/graphics/color/PDDeviceCMYK.java#L60]
> - {{volatile}} set isn't the last statement in the block.
> *
> [https://github.com/apache/pdfbox/blob/e409f25009702be8889ce586b8f6dd7274201f0a/pdfbox/src/main/java/org/apache/pdfbox/pdmodel/graphics/color/PDDeviceRGB.java#L48]
> - {{volatile}} set isn't the last statement in the block.
> *
> [https://github.com/apache/pdfbox/blob/e409f25009702be8889ce586b8f6dd7274201f0a/fontbox/src/main/java/org/apache/fontbox/ttf/TrueTypeFont.java#L162-L167]
> - {{initialized}} isn't {{volatile}}
> *
> [https://github.com/apache/pdfbox/blob/947966ea830ff91c61a6740ca1787eb795b5ca95/pdfbox/src/main/java/org/apache/pdfbox/pdmodel/font/encoding/Encoding.java#L132-L149]
> - {{names}} isn't {{volatile}} and the second check is missing (which might
> be harmless)
> *
> [https://github.com/apache/pdfbox/blob/6c8526bab8b7ca399721e067d065c1f272f97644/pdfbox/src/main/java/org/apache/pdfbox/pdmodel/font/Standard14Fonts.java#L172-L190]
> - {{fonts}} isn't even locked and it's a vanilla hashmap that can be
> modified by other threads.
> *
> [https://github.com/apache/pdfbox/blob/7984a52ad4d475886568593614ce566a88bf6bdd/pdfbox/src/main/java/org/apache/pdfbox/cos/COSName.java#L632-L637]
> - (tricky to see, but the constructor adds itself to the map) - the map
> isn't locked before the blind {{put}}, so unclear which invocation "wins"
> (not sure if this is an issue, logic wise).
> *
> [https://github.com/apache/pdfbox/blob/61d6a53eacdee6a40d352509105e1c8d51cfb5dc/pdfbox/src/main/java/org/apache/pdfbox/pdmodel/font/FontMapperImpl.java#L415-L419]
> - {{fontProvider}} isn't volatile, though is used as a double checked lock
> marker. (Not sure if this an issue, concurrency wise).
> Fixing these one-by-one is possible and what I started doing around
> [https://github.com/apache/pdfbox/pull/90] - but would it make sense to make
> a class that does this properly? Guava has {{Suppliers.memoize}} which does
> the right thing - it's trivial to make an equivalent without the dep in
> pdfbox as well if necessary.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
