Updated domain.xml file management + CI for windows #25205
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tempts Signed-off-by: David Matějček <[email protected]>
Signed-off-by: David Matějček <[email protected]>
- When something threw an exception, it continued without this locking. - It used different file for locking, because domain.xml was renamed, deleted. - Two test classes failed on some systems - Arjan's Mac, David's Ubuntu HP, but passed on CI and David's Ubuntu Dell. Differences were also on other systems. We have never found exact reason why. - Basically I believe that any implementation will be unreliable, because we do access domain.xml file from different processes, move it, write and read it and the access cannot use any kind of locking except native hard drive and whatever is provided by the system. But once we delete the file, we lose its handle in JVM and all locks are gone. The order of operations is unspecified, we can control just our own work with the given path. - So ... here I use just the read lock which should protect the file when some thread is reading it. File manipulation uses NIO. - Is it everything ...? Signed-off-by: David Matějček <[email protected]>
dmatej
added
bug
Signed-off-by: David Matějček <[email protected]>
Signed-off-by: David Matějček <[email protected]>
- Now throws exceptions when something goes wrong - Doesn't ignore failed renames - AddLibraryCommand uses result of this method for the classloader Signed-off-by: David Matějček <[email protected]>
dmatej
changed the title
- Were missing on some places - That fact broke some tests on some Windows file systems which used "suspicious" file path/uri strings refused by java while the string should have always been an URI. - I pushed the conversion from getters to setters to make it clear what kind of URI/path was used - then we can do more appropriate coversion earlier and just once (however GlassFishProperties are still just a bunch of strings so there will still be redundant conversions for now). Signed-off-by: David Matějček <[email protected]>
Signed-off-by: David Matějček <[email protected]>
- If we would experience some issues with race conditions over domain.xml, we have to find better solution, because this did not pass its own tests on some file systems (my HP Victus with Kubuntu 24.10 and ext4, virtual Windows Server 2022 provided by GitHub Actions, maybe other). Signed-off-by: David Matějček <[email protected]>
Signed-off-by: David Matějček <[email protected]>
dmatej
requested review from
OndroMih,
arjantijms,
hs536,
avpinchuk and
kaido207
arjantijms
approved these changes
Nov 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Story
I am using a backup laptop now, and to my surprise some tests started failing. File locking tests here fail just like on Arjan's Mac last year, but I am on Kubuntu 24.10, fs ext4. On Kubuntu 24.04 with ext4 they always passed, same on Jenkins CI, so it seems it might be something around CPU, OS, or NVME firmware, I have no idea.
I had doubts if this removal of ManagedFile would not break something on Windows, so I decided to add windows-based CI build to check as I don't have any Windows machine available at this moment.
Changes
New GitHub Action for all pull requests - build on Windows.
Original impl of locking: when something threw an exception, it continued without this locking.
It used different file for locking, because domain.xml was renamed, deleted.
Two test classes failed on some systems - Arjan's Mac, David's Ubuntu HP, but passed on Jenkins CI and David's Ubuntu Dell. Differences were also on other systems. We have never found exact reason why.
Basically I believe that any implementation will be unreliable, because we do access domain.xml file from different processes, move it, write and read it and the access cannot use any kind of locking except native hard drive and whatever is provided by the system. But once we delete the file, we lose its handle in JVM and all locks are gone. The order of operations is unspecified, we can control just our own work with the given path.
Fixed or disabled tests which failed on Windows.