You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@jena.apache.org by GitBox <gi...@apache.org> on 2020/03/13 22:09:06 UTC
[GitHub] [jena] intrigus-lgtm opened a new pull request #709: Add volatile
to make method thread-safe
intrigus-lgtm opened a new pull request #709: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709
The old documentation said this:
// Does not matter about race condition here because the same Set should be
// created on any call to generateAcceptedParams.
But this does not make the race in any way benign.
Since we are doing racy reads, it is in fact possible that after
any of the `if(acceptedParams_ == null)` evaluates to true
- and replaces acceptedParams_ with a non-null value -
`return acceptedParams_` can still return null!
Other possible ways to solve this problem are detailed [here](https://shipilev.net/blog/2014/safe-public-construction/#_safe_publication).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on issue #709: JENA-1866: Add volatile to make
method thread-safe
Posted by GitBox <gi...@apache.org>.
afs commented on issue #709: JENA-1866: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709#issuecomment-601080212
[JENA-1866](https://issues.apache.org/jira/browse/JENA-1866)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs edited a comment on issue #709: Add volatile to make
method thread-safe
Posted by GitBox <gi...@apache.org>.
afs edited a comment on issue #709: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709#issuecomment-599228455
I'm not a JMM expert either - it's good to thrash out the details. We both learn - the code is better. Unit testing concurrent code is insufficient - thinking and talking about concurrent code is needed.
> The thread 2 then may still not see a completely initialized Set.
meaning the set object from thread 1 is not visible properly yet? OK - I can see that now.
Making `acceptedParams_` volatile puts in a happens-before point (write to a volatile).
I was hoping to avoid a volatile - more a style thing, the code is trying to create an immutable object while allowing `fusekiParams()` or `customParams()` to be overridden. That's feature not currently used but is there to allow additional params to be added by customizations.
The fact that two objects can be created does not matter. If thread 1 creates a set, and uses it, and thread 2 sees null still, creates its own object and uses it, eventually overwriting `acceptedParams_` is not a problem (it is always the same set contents).
Is there a better way to achieve subclass overrides to provide `customParams()`? If it weren't for that, everything could be done in class statics.
Otherwise, volatile it is.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs merged pull request #709: JENA-1866: Add volatile to
make method thread-safe
Posted by GitBox <gi...@apache.org>.
afs merged pull request #709: JENA-1866: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] intrigus-lgtm commented on issue #709: Add volatile to make
method thread-safe
Posted by GitBox <gi...@apache.org>.
intrigus-lgtm commented on issue #709: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709#issuecomment-599111184
The problem with the current code is this:
Even if we write to `acceptedParams_` [here](https://github.com/apache/jena/pull/709/files#diff-f3592fdcfd01af694d9a604a977a972fL102),
https://github.com/apache/jena/pull/709/files#diff-f3592fdcfd01af694d9a604a977a972fL105 can **still** return `null`.
The Java Memory Model (JMM) evaluates every read in isolation, such that
if there are two reads, the first read could see a non-null value and the second read could see a null value.
So consider this:
(Time progresses from 1 to 4)
Thread 1
1. calls `acceptedParams(action)`
2. writes to `acceptedParams_`
Thread 2
3. Calls `acceptedParams(action)`
4. Sees that `acceptedParams_` is not null and returns it
The thread 2 then may **still not** see a completely initialized Set.
Just because thread 2 sees `acceptedParams` being not null, it does not have to see any of the stores, that add the values to the Set.
This can only be fixed by adding volatiles or using one of the methods detailed [here](https://shipilev.net/blog/2014/safe-public-construction/#_safe_publication).
**Caveat: I'm no JMM expert, so take everything I say with some caution**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on issue #709: Add volatile to make method
thread-safe
Posted by GitBox <gi...@apache.org>.
afs commented on issue #709: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709#issuecomment-599424818
Class intialization does not work in this case. {{generateAcceptedParams}} is calling object methods so {{static class}} doesn't compile. The protected override that makes this different.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on issue #709: Add volatile to make method
thread-safe
Posted by GitBox <gi...@apache.org>.
afs commented on issue #709: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709#issuecomment-599228455
I'm not a JMM expert either - it's good to thrash out the details. We both learn - the code is better. Unit testing concurrent code is insufficient - thinking and talking about concurrent code is needed.
> The thread 2 then may still not see a completely initialized Set.
meaning the set object from thread 1 is not visible properly yet? OK - I can see that now.
Making `acceptedParams_` volatile puts in a happens-before point (write to a volatile).
I was hoping to avoid a volatile - more a style thing, the code is trying to create an immutable object while allowing `fusekiParams()` or `customParams()` to be overridden. That's feature not currently used but is there to allow additional params to be added by customizations.
The fact that two objects can be created does not matter. If thread 1 creates a set, and uses it, and thread 2 sees null still, creates its own object and uses it, eventually overwriting `acceptedParams_` is not a problem (it is always the same set contents).
Is there a better way to achieve subclass overrides to provide `customParams()`? If it weren't for that, everything could be done in class statics.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] intrigus-lgtm commented on issue #709: JENA-1866: Add
volatile to make method thread-safe
Posted by GitBox <gi...@apache.org>.
intrigus-lgtm commented on issue #709: JENA-1866: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709#issuecomment-601110659
Thank you!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] intrigus-lgtm commented on issue #709: Add volatile to make
method thread-safe
Posted by GitBox <gi...@apache.org>.
intrigus-lgtm commented on issue #709: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709#issuecomment-599796918
My bad.
(Didn't try it)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] intrigus-lgtm commented on issue #709: Add volatile to make
method thread-safe
Posted by GitBox <gi...@apache.org>.
intrigus-lgtm commented on issue #709: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709#issuecomment-599237130
I like this code the most, it should also be the fastest:
(This should also work with subclasses overriding `fusekiParams()` or `customParams()`, I think)
````java
protected Collection<String> acceptedParams(HttpAction action) {
return Holder.instance;
}
private static class Holder {
public static final Set<String> instance = generateAcceptedParams();
}
````
Explanation by [Aleksey Shipilëv](https://shipilev.net/):
> This is how it works spec-wise:
>
> 1. Class initialization is done under the lock, as per JLS 12.4.2. Class initialization lock provides the mutual exclusion during the class initialization, that is, only a single thread can initialize the static fields.
>
> 2. The release of class initialization lock plays the necessary role in establishing the happens-before relationships between the actions in static initializers and any users of the static fields. Naively speaking, the propagation of memory effects requires any reader of static field to acquire the class initialization lock first, but JLS allows to elide that locking, if the memory effects are still maintained. Indeed, modern VMs do this optimization routinely.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on issue #709: Add volatile to make method
thread-safe
Posted by GitBox <gi...@apache.org>.
afs commented on issue #709: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709#issuecomment-599233677
As a thought experiment, do you think this would work? A two step setting of the eventual `params_`: the volatile assignment will cause a "happens-before", it is then assigned to the class member which is used for the on-entry test:
private volatile Set<String> paramsv_ = null;
private Set<String> params_ = null;
protected Collection<String> delayedParams() {
if ( params_ != null )
return params_;
synchronized(this) {
if ( paramsv_ == null )
paramsv_ = generateAcceptedParams();
params_ = paramsv_;
return paramsv_;
}
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on issue #709: Add volatile to make method
thread-safe
Posted by GitBox <gi...@apache.org>.
afs commented on issue #709: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709#issuecomment-599960212
@intrigus-lgtm - did you observe a problem happening for real? If so, is there a characteristic to the Fuskei usage that pushed it into problems?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on issue #709: Add volatile to make method
thread-safe
Posted by GitBox <gi...@apache.org>.
afs commented on issue #709: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709#issuecomment-599087715
This isn't the full case of creating a unique singleton. If it were, a volatile would be needed.
It is ensuring there is late initialization so the protected methods can be overridden but any object generated by `generateAcceptedParams()` is valid.
It does not have to be a uniquely created object - in that way, it differs from the singleton pattern.
All the code does is create several objects, all of which are valid, and keeps one (the last assignment) eventually. It does not matter which on it keeps. It does not matter if threads use an object that will be dropped by a later or overlapping assignment. All the object are "same value" which is all that matters for a set.
```
// Does not matter about race condition here because the same Set should be
// created on any call to generateAcceptedParams.
```
A set is "the same" if it has the same values in it.
If two happen and overlap, then one pointer will overwrite the other but it's overwriting with an equivalent object and assignment of object references is atomic in Java.
In the singleton pattern, it is a stronger condition - trying to create a single object once and the singleton may have varying state (e.g a cache).
Thanks for checking!
Is there a way to make the comments clearer?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs edited a comment on issue #709: Add volatile to make
method thread-safe
Posted by GitBox <gi...@apache.org>.
afs edited a comment on issue #709: Add volatile to make method thread-safe
URL: https://github.com/apache/jena/pull/709#issuecomment-599424818
Class intialization does not work in this case. `generateAcceptedParams` is calling object methods so `static class` doesn't compile. The protected override that makes this different.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org