8362252: [lworld] Possible synchronization issue in jdk/internal/jimage/ImageReader.java #1828
Conversation
|
👋 Welcome back david-beaumont! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
|
||
| synchronized (OPEN_FILES) { | ||
| ReaderKey key = new ReaderKey(imagePath, previewMode); | ||
| SharedImageReader reader = OPEN_FILES.get(key); |
There was a problem hiding this comment.
I changed the name of this for clarity. It's a bit confusing to have two "image reader" instances in scope with one called "image" and one called just "reader".
| throw new IOException("image file not found in open list"); | ||
| } | ||
| } | ||
| super.close(); |
There was a problem hiding this comment.
I will double check if moving this outside the synchronization of OPEN_FILES is an issue.
The underlying BasicImageReader using a file channel, which is close (with locking) in this close() method.
The vague worry I have is that now the outter OPEN_FILES lock isn't held, we can get a race where the same file has a file channel being closed as a new one is being opened, and I'm not 100% sure I know if that's safe.
Moving this back into the OPEN_FILES lock is possible, but leaves this code doing more work with the locks held, which I'm inclined to avoid if possible.
| sharedReader = new SharedImageReader(imagePath, byteOrder, previewMode); | ||
| OPEN_FILES.put(key, sharedReader); | ||
| } else if (sharedReader.getByteOrder() != byteOrder) { | ||
| throw new IOException("\"" + sharedReader.getName() + "\" is not an image file"); |
There was a problem hiding this comment.
Its pre-existing, but ...
Testing the byteOrder and then throwing an exception that says it is not an Image file doesn't make sense.
2 participants
Tidying up syncrhonization around shared image.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1828/head:pull/1828$ git checkout pull/1828Update a local copy of the PR:
$ git checkout pull/1828$ git pull https://git.openjdk.org/valhalla.git pull/1828/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1828View PR using the GUI difftool:
$ git pr show -t 1828Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1828.diff
Using Webrev
Link to Webrev Comment