[
https://issues.apache.org/jira/browse/PDFBOX-5056?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17254760#comment-17254760
]
Mike Kaplinskiy edited comment on PDFBOX-5056 at 12/25/20, 7:43 AM:
--------------------------------------------------------------------
[~lars t] {{volatile}} isn't a machine instruction - it's a compiler
instruction. Specifically, it prevents the JVM from taking the double checked
locking code above:
{code:java}
private boolean doneInit = false;
if (!doneInit) {
synchronized (this) {
if (!doneInit) {
... do init
doneInit = true;
}
}
} {code}
and turning it into, effectively:
{code:java}
private boolean doneInit = false;
if (!doneInit) {
synchronized (this) {
dontInit = true;
... do init
}
} {code}
Normally the JVM can do this kind of transformation when it knows the value of
{{doneInit}} isn't used in the code in the middle & there is an
instruction-level parallelism to be gained.
In the correct version above, other variables are protected by the {{volatile}}
set - the volatile set is guaranteed to not be reordered relative to other
instructions above or below it - which means anyone bypassing the
{{synchronized}} block will already see all the results. It's also why you
might not want to make something play a double-role as your {{volaitile}}
marker - it prevents JVM optimizations. You can read more about the Java memory
model @
[https://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#reordering] .
To look at it from another perspective - if {{awtColorSpace}} needed to be
{{volatile}} then simple single-checked locking wouldn't work correctly, i.e.
{code:java}
private ICC_ColorSpace awtColorSpace;
synchronized (this) {
if (awtColorSpace == null) {
return;
}
awtColorSpace = ...
}{code}
Separately though, using an object rather than a primitive as a {{volatile}}
might end poorly - I do not know what Java's guarantees are around a
half-constructed object being stored in the pointer (in C++ it definitely
doesn't work).
was (Author: mikekap):
[~lars t] {{volatile}} isn't a machine instruction - it's a compiler
instruction. Specifically, it prevents the JVM from taking the double checked
locking code above:
{code:java}
private boolean doneInit = false;
if (!doneInit) {
synchronized (this) {
if (!doneInit) {
... do init
doneInit = true;
}
}
} {code}
and turning it into, effectively:
{code:java}
private boolean doneInit = false;
if (!doneInit) {
synchronized (this) {
dontInit = true;
... do init
}
} {code}
Normally the JVM can do this kind of transformation when it knows the value of
{{doneInit}} isn't used in the code in the middle & there is an
instruction-level parallelism to be gained.
In the correct for above, other variables are protected by the {{volatile}} set
- the volatile set is guaranteed to not be reordered relative to other
instructions above or below it - which means anyone bypassing the
{{synchronized}} block will already see all the results. It's also why you
might not want to make something play a double-role as your {{volaitile}}
marker - it prevents JVM optimizations. You can read more about the Java memory
model @
[https://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#reordering] .
To look at it from another perspective - if {{awtColorSpace}} needed to be
{{volatile}} then simple single-checked locking wouldn't work correctly, i.e.
{code:java}
private ICC_ColorSpace awtColorSpace;
synchronized (this) {
if (awtColorSpace == null) {
return;
}
awtColorSpace = ...
}{code}
Separately though, using an object rather than a primitive as a {{volatile}}
might end poorly - I do not know what Java's guarantees are around a
half-constructed object being stored in the pointer (in C++ it definitely
doesn't work).
> 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]
