Skip to content

Avoid cached data between plugin generation #31308

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: integration
Choose a base branch
from
Open

Avoid cached data between plugin generation #31308

wants to merge 3 commits into from

Conversation

emanuel2258
Copy link

@emanuel2258 emanuel2258 commented Apr 18, 2025 •
edited
Loading

Created a private static method that re‑initializes every static field used to hold merge state and it's used to ensuring each invocation begins with a clean slate, regardless of any prior merges.

Fixes #31263

@emanuel2258 emanuel2258 added in:Web Components release bug This bug is present in a released version of Open Liberty team:Sirius labels Apr 18, 2025
@emanuel2258 emanuel2258 self-assigned this Apr 18, 2025
@emanuel2258 emanuel2258 requested a review from pnicolucci April 18, 2025 14:25
Created a private static method that re‑initializes every static fiel…
afc474b 
…d used to hold merge state and it's used to ensuring each invocation begins with a clean slate, regardless of any prior merges.
@emanuel2258
Copy link
Author

emanuel2258 commented Apr 18, 2025 •
edited by LibbyBot
Loading

!build
(view Open Liberty Personal Build - ❌ completed with errors/failures.)
spawn.fullfat.buckets=com.ibm.ws.webserver.plugin.utility_fat

Note: Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • 1 product code files were changed.
  • Please describe in a separate comment how you tested your changes.

pnicolucci
pnicolucci requested changes Apr 18, 2025
View reviewed changes
Copy link
Member

@pnicolucci pnicolucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static was added here: #24627 we need to understand why that was. If these variables don't need to be static we should fix that. If we have test failures we need to understand why and determine a solution.

@volosied
Copy link
Member

I encounter failures when I change private PluginInfo[] plugins variable to non-static.

A clue as to why this variable needs to be static is how the merge tool is used, via a singleton-ish pattern:

PluginMergeToolImpl toolInstance = new PluginMergeToolImpl(); within the merge(String argv[]) method. After toolInstance is created, plugins = new PluginInfo[fileList.length]; is made.

Another place to look at is where plugins = new PluginInfo[files.length - 1]; was initialized. Before it was in the loadData method, but now it's within merge method.

This plugins object would then be accessible by toolInstance since it's static. If it's not static, then you'll hit NPEs (Caused by: java.lang.NullPointerException: Cannot read the array length because "this.plugins" is null).

I have a branch here with some local changes for testing: https://github.com/volosied/open-liberty/tree/TS018131209-vlad

The 789a331 commit has all tests passing while the c9fc73d commit has only the testEndpointTest3 test failing.

@emanuel2258
Copy link
Author

emanuel2258 commented Apr 25, 2025 •
edited
Loading

I made a new commit to the PR. I reverted a lot of the static variables to instance state instead of resetting the state as a work around. When I say reverted I mean compared to what it was prior to PR #24627. There is only one variable that was left static sharedPlugins. If I modified it to non-static it would cause problems in one of the tests in com.ibm.ws.webserver.plugin.utility_fat, so had the potential to regress. But if I leave it as static it still causes problems for the customer’s issue because I would still get cached data. In order to get around this I had to add sharedPlugins = new ArrayList<PluginInfo>(); at the beginning of the merge function. Please take a look when you can and let me know if you want me to modify anything else.

pnicolucci
pnicolucci requested changes Apr 29, 2025
View reviewed changes
Copy link
Member

@pnicolucci pnicolucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a test added as well

@emanuel2258 emanuel2258 mentioned this pull request May 2, 2025
Open
pnicolucci
pnicolucci reviewed May 2, 2025
View reviewed changes
....ibm.ws.http.plugin.merge/src/com/ibm/ws/http/plugin/merge/internal/PluginMergeToolImpl.java
@@ -188,7 +192,7 @@ private void printMergedCopy(String output) throws IOException, ParserConfigurat
DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
DocumentBuilder parser = dbf.newDocumentBuilder();
Document mergeDoc = parser.newDocument();
final Comment comment = mergeDoc.createComment(" This config file was generated by pluginUtility merge v1.0.73 on " +
final Comment comment = mergeDoc.createComment(" This config file was generated by pluginUtility merge v1.0.75 on " +
dateTimeFormatter.format(ZonedDateTime.now()) + " ");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how this version is determined and why we go from 73 to 75, can you clarify?

pnicolucci
pnicolucci reviewed May 2, 2025
View reviewed changes
....ibm.ws.http.plugin.merge/src/com/ibm/ws/http/plugin/merge/internal/PluginMergeToolImpl.java
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2013, 2023 IBM Corporation and others.
* Copyright (c) 2013, 2025 IBM Corporation and others.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also remove the "contributors" section of the copyright statement. It is no longer needed. I try to clean these up as we touch files.

pnicolucci
pnicolucci reviewed May 2, 2025
View reviewed changes
Copy link
Member

@pnicolucci pnicolucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaning up your commit history would be great, there are commits there with descriptions that don't make sense any longer as the design of the fix has changed since some of the commits were added. Adding a test case as we've discussed would be excellent as well.

@pnicolucci pnicolucci self-requested a review May 2, 2025 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@volosied volosied volosied left review comments

@pnicolucci pnicolucci Awaiting requested review from pnicolucci

Requested changes must be addressed to merge this pull request.

Assignees

@emanuel2258 emanuel2258

Labels
CLA Signed in:Web Components release bug This bug is present in a released version of Open Liberty team:Sirius
Projects
Web Tier Team
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Plugin Config generation caching issues
4 participants
@emanuel2258 @LibbyBot @volosied @pnicolucci