-
Notifications
You must be signed in to change notification settings - Fork 3k
ClosedFileSystemException or NullPointerException thrown when SharedArchivePathTree is opened and closed concurrently #48221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @ppalaga
This comment has been minimized.
This comment has been minimized.
SharedArchivePathTree is opened and closed concurrently, fix quarkusio#48220
@geoand did you do anything else than rebase? I wanted to have a closer look at the failed tests today. |
Only rebase onto |
This comment has been minimized.
This comment has been minimized.
I'm gonna re-run it again. |
} | ||
try { | ||
this.lastOpen = new SharedOpenArchivePathTree(openFs()); | ||
lastOpen = this.lastOpen = new SharedOpenArchivePathTree(openFs()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not 100% sure what the intention behind the split into SharedArchivePathTree
, SharedOpenArchivePathTree
and CallerOpenPathTree
is. A sentence of JavaDoc on each of those classes would help.
My hypothesis is that the author wanted to allow multiple instances of SharedOpenArchivePathTree
per SharedArchivePathTree
where:
- The one stored in
lastOpen
is the preferred and longer living one - All others are closed immediately when they are disposed via
close()
. - Calling
openFs()
multiple times on a single file (pernew SharedOpenArchivePathTree(openFs())
) does not make the underlyingZipFilesystemProvider
throw aFileSystemAlreadyExistsException
, because perhaps theSharedArchivePathTree.CACHE
somehow prevents it? Or what else is expected to happen in such situation?
When I saw SharedArchivePathTree.lastOpen
writes and reads being protected by OpenArchivePathTree.lock
in SharedOpenArchivePathTree
, I thought it was a mistake not to protect also writing to lastOpen
in SharedArchivePathTree.open()
:
try {
lastOpen = this.lastOpen = new SharedOpenArchivePathTree(openFs());
} catch (IOException e) {
throw new UncheckedIOException(e);
}
But then, I figured out that the reads/writes from SharedOpenArchivePathTree
are protected by per SharedOpenArchivePathTree
lock anyway, so multiple instances of SharedOpenArchivePathTree
can still race on accessing it. So perhaps it was not really intended not to protect SharedArchivePathTree.lastOpen
by those locks at all but rather, those concurrent reads/writes to SharedArchivePathTree.lastOpen
are by design and not considered harmful. Could please anybody confirm that? Git blame shows @aloubyansky as an author but hard to say if he wrote it originally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SharedArchivePathTree
has a bit of a Javadoc explaining the idea behind it.
SharedOpenArchivePathTree
is the OpenArchivePathTree
impl of it.
CallerOpenPathTree
is basically shielding the SharedOpenArchivePathTree
from the same caller calling close()
multiple times on it and so from decrementing the user counter.
You're right about lastOpen
not being well protected though. I'll have another look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SharedArchivePathTree
has a bit of a Javadoc explaining the idea behind it.
I was very thankful to see it :)
SharedOpenArchivePathTree
is theOpenArchivePathTree
impl of it.
CallerOpenPathTree
is basically shielding theSharedOpenArchivePathTree
from the same caller callingclose()
multiple times on it and so from decrementing the user counter.
Do we really need both Open* and Caller* flavors (and all abstraction layers above)?
Does OpenPathTree have to inherit from PathTree?
All those inheritance, inner-class and delegation links make it really hard to understand what is going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does OpenPathTree have to inherit from PathTree?
Yeah, OpenPathTree is something that needs to be closed but it otherwise a PathTree.
The whole complication is around caching and concurrent access to the same open archive.
Unfortunately, the test rarely fails for me w/o the fix.
This comment has been minimized.
This comment has been minimized.
Status for workflow
|
@aloubyansky do we merge this one or do you want to have another look before we merge? |
I'd merge it and also have another look. I have a few things to look into, so I wouldn't delay it since it appears to fix an issue. I didn't expect the CI failures though. They might be unrelated but I don't think we had a clean CI for this PR. |
I also struggled to make the included test fail w/o the fix. But it's kinda expected for it to be tricky to fail consistently. |
ClosedFileSystemException
orNullPointerException
thrown whenSharedArchivePathTree
is opened and closed concurrently #48220