8362252: [lworld] Possible synchronization issue in jdk/internal/jimage/ImageReader.java#1828
8362252: [lworld] Possible synchronization issue in jdk/internal/jimage/ImageReader.java#1828david-beaumont wants to merge 2 commits intoopenjdk:lworldfrom
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. |
Webrevs
|
|
|
||
| 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.
There was a problem hiding this comment.
Roger, I'd still want a confirmation of the safety around channel opening/closing on the same underlying file.
| 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.
There was a problem hiding this comment.
That's a very good point.
|
@david-beaumont This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
@david-beaumont This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
@david-beaumont This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
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