JENKINS-31932: getJDKs/setJDKs synchronization done using a lock object

[lukaszkarnasiewicz]

https://issues.jenkins-ci.org/browse/JENKINS-31932

[oleg-nenashev]

Sounds reasonable. 👍

[daniel-beck]

@daspilker FYI

[daniel-beck]

LGTM.

[daspilker]

Hm, I still don't see why this needs to be synchronized at all. See the original discussion at #1692 (comment)

[jtnord]

I think I agree with @daspilker - all that is needed is the jdks to be volatile AFAICT.

[lukaszkarnasiewicz]

Well, you have a mutable field jdks *in the *Jenkins *class so you have to
make sure that access to it is "synchronized" in some way. I agree that in
this case for the time being *volatile *would be sufficient. Would you like
me to modify this pull request to use *volatile instead of a synchronised
block?

wt., 22.12.2015 o 11:55 użytkownik James Nord notifications@github.com
napisał:

I think I agree with @daspilker https://github.com/daspilker - all that
is needed is the jdks to be volatile AFAICT.

—
Reply to this email directly or view it on GitHub
#1947 (comment).

Pozdrawiam,
Łukasz Karnasiewicz

[jtnord]

I have a feeling that volatile should perform better than locking - but it is only a feeling. I would wait to see if others have any opinion on this.
Personally I would prefer that the List is replaced by a thread safe implementation but there is not one that has an atomic clearAndSet operation that I am aware of.

[daspilker]

This will break any existing code that modifies the list returned by getJDKs(), e.g. https://github.com/jenkinsci/jenkins/blob/jenkins-1.643/test/src/test/java/hudson/model/ProjectTest.java#L332

Why hasn't this been tested by the pull request builder? This is not ready for merge.

[jtnord]

given ArrayList is not thread safe - you can consider that "a good thing(tm)"