CI: also run on Windows and MacOS, fix OF-3208, OF-3209#2986
Open
guusdk wants to merge 5 commits intoigniterealtime:mainfrom
Open
CI: also run on Windows and MacOS, fix OF-3208, OF-3209#2986guusdk wants to merge 5 commits intoigniterealtime:mainfrom
guusdk wants to merge 5 commits intoigniterealtime:mainfrom
Conversation
guusdk
force-pushed
the
CI-include-Windows
branch
2 times, most recently
from
18be7b3 to
1611ca2
Compare
guusdk
added
backport 5.0
guusdk
force-pushed
the
CI-include-Windows
branch
3 times, most recently
from
3bab55e to
594df80
Compare
Member
Author
|
I have added this as a way to detect issues as reported in and fixed by #2982 However, running the build on Windows and MacOS does not reproduce these issues (they can be reproduced by including a space character in the working directory - I've attempted to do that, but there doesn't seem to be an easy/maintainable way to do that in GitHub workflows - at least none that I can find). With that, do we want to include Windows and MacOS as CI runners? They aren't exposing the issue that I had hoped, after all. With more runners, the chance of build failures because of flapping tests increases. |
guusdk
force-pushed
the
CI-include-Windows
branch
from
d4b7b88 to
594df80
Compare
akrherz
force-pushed
the
CI-include-Windows
branch
from
594df80 to
9c2c1b9
Compare
Member
|
MacOS failure filed as https://igniterealtime.atlassian.net/browse/OF-3119 |
guusdk
added
backport 5.0
guusdk
force-pushed
the
CI-include-Windows
branch
from
9c2c1b9 to
14d06bf
Compare
Member
Author
|
Rebased. |
guusdk
force-pushed
the
CI-include-Windows
branch
2 times, most recently
from
7b31f81 to
0b43a81
Compare
guusdk
force-pushed
the
CI-include-Windows
branch
from
0b43a81 to
6cdaea4
Compare
guusdk
removed
the
backport 5.0
guusdk
changed the title
Member
Author
|
Windows failure filed as OF-3208 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands Openfire’s CI coverage to include Windows and macOS (in addition to Linux) and addresses Windows-specific file locking issues that can prevent plugin directories from being deleted (OF-3208).
Changes:
- Run the CI build job on
ubuntu-latest,windows-latest, andmacos-latest, and adjust artifact naming/coverage conditions accordingly. - Improve plugin unload cleanup by retrying deletion with GC/finalization and falling back to
deleteOnExit()registration when deletion remains impossible (Windows file locks). - Reduce unit test flakiness by replacing time-based sleeps with a “wait until done” helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
xmppserver/src/test/java/org/jivesoftware/openfire/container/PluginLoadingComplexHierarchyTest.java |
Adjusts teardown to better tolerate Windows file-lock behavior (plus a minor comment typo). |
xmppserver/src/test/java/org/jivesoftware/openfire/archive/ArchiverTest.java |
Replaces a timing-based sleep with a deterministic “wait until archiving is done” helper call. |
xmppserver/src/main/java/org/jivesoftware/openfire/container/PluginManager.java |
Adds Windows-oriented cleanup logic for plugin extracted directories and tweaks unload ordering to reflect logical state sooner. |
.github/workflows/continuous-integration-workflow.yml |
Adds an OS matrix (Linux/Windows/macOS) and updates artifact naming/conditions for coverage and reuse. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ver/src/test/java/org/jivesoftware/openfire/container/PluginLoadingComplexHierarchyTest.java
Outdated
Show resolved
Hide resolved
xmppserver/src/main/java/org/jivesoftware/openfire/container/PluginManager.java
Outdated
Show resolved
Hide resolved
xmppserver/src/main/java/org/jivesoftware/openfire/container/PluginManager.java
Outdated
Show resolved
Hide resolved
xmppserver/src/main/java/org/jivesoftware/openfire/container/PluginManager.java
Show resolved
Hide resolved
guusdk
force-pushed
the
CI-include-Windows
branch
from
534b37c to
0977396
Compare
Member
Author
|
I've added code changes resulting from the code review by amending it to the original commit. |
guusdk
changed the title
guusdk
force-pushed
the
CI-include-Windows
branch
2 times, most recently
from
24ba58c to
1b8e3a5
Compare
Member
Author
guusdk
force-pushed
the
CI-include-Windows
branch
3 times, most recently
from
4ff1167 to
54eeefc
Compare
As we're producing binaries for Windows, and that OS notably differs from Linux in things like the representation of a path on the file system, we should run CI in a Windows environment in addition to running it on Linux-based environments. While we're at it, we might as well also include MacOS.
On Windows, the JVM holds mandatory file locks on JAR/class files opened by a plugin's class loader. These locks are not released until the underlying ZipFile/JarFile handles are garbage-collected, which prevented the extracted plugin directory from being deleted during unload. Because the cleanup of internal data structures (pluginsLoaded, childPluginMap, parentPluginMap) was sequenced after directory deletion, isLoaded() would incorrectly return true for plugins that had been unloaded, causing testDelete_Node_A and testReload_Node_A to fail on Windows CI. Changes: - Move pluginsLoaded/childPluginMap/parentPluginMap removal to immediately after doDestroyClassLoader, so that isLoaded() reflects logical state regardless of whether physical file deletion succeeds. - Add System.runFinalization() alongside the existing System.gc() hints in doDestroyExtractedFiles to improve the likelihood of lock release between deletion attempts. Note: System.runFinalization() is deprecated since Java 18 and will need to be removed when support for it is dropped. - Register all remaining files and directories with File.deleteOnExit() if the extracted plugin directory still exists after all retry attempts.
…ned directories On Windows, mandatory JVM file locks can prevent the extracted plugin directory from being deleted during unload. This causes two problems: 1. A deleted plugin whose JAR is gone may be reloaded from its orphaned directory. Fixed by filtering the directory stream to skip directories without a corresponding JAR/WAR file. 2. A reloaded plugin may be re-extracted from a stale directory if the previous directory deletion failed. Fixed by comparing JAR and directory modification times in unzipPlugin, forcing re-extraction when the JAR is newer than the existing directory. Resolves test failures in PluginLoadingSimpleHierarchyTest and PluginLoadingComplexHierarchyTest on Windows.
guusdk
force-pushed
the
CI-include-Windows
branch
from
54eeefc to
0b95f07
Compare
Previously, a plugin was marked to be reloaded by having its 'last modified' date reset. This is an operation that's not guaranteed to be successful on all operating systems. In this commit, a more explicit flag is used.
guusdk
force-pushed
the
CI-include-Windows
branch
from
72d1c87 to
02b7e57
Compare
This modifies a unit test to no longer determine based on file attributes (last modified timestamp) if a plugin got (re)loaded. These attributes are not reliably set on all operating systems. The new approach registers an event listener instead.
This file contains hidden or 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
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.
As we're producing binaries for Windows, and that OS notably differs from Linux in things like the representation of a path on the file system, we should run CI in a Windows environment in addition to running it on Linux-based environments.
While we're at it, we might as well also include MacOS.