Skip to content
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

Restore wstx-asl-3.2.9.jar to WAR #5604

Merged
merged 1 commit into from
Jul 5, 2021
Merged

Restore wstx-asl-3.2.9.jar to WAR #5604

merged 1 commit into from
Jul 5, 2021

Conversation

basil
Copy link
Member

@basil basil commented Jul 2, 2021

Partial revert of #5559, which removed stax-api-1.0.1.jar, stax-api-1.0-2.jar, and wstx-asl-3.2.9.jar from the WAR under the premise that StAX was now provided by the Java Platform. While removing the first two JARs caused no problems, removing wstx-asl-3.2.9.jar (a StAX implementation, in particular the very old Woodstox 3.2.9) was apparently too aggressive and did cause jenkinsci/azure-storage-plugin#194. As a hotfix, restore the working status quo by re-adding wstx-asl-3.2.9.jar to the WAR. I verified that this chases away jenkinsci/azure-storage-plugin#194. In the future, it would be good to rationalize our usage of StAX/Jackson/Woodstox across core and the Jackson 2 API plugin and eliminate this old dependency from core. In particular, we shouldn't need to ship Woodstox in core at all, but rather the Jackson 2 API plugin should provide a modern version of jackson-databind-xml and Woodstox to all dependent plugins. That is a bit more work and I couldn't complete it in a few hours. In the meantime the regression in core needs to be fixed ASAP.

Proposed changelog entries

Fix a regression in 2.298 where some plugins (including Azure Storage) could not correctly parse streaming XML output.

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@basil basil added dependencies Pull requests that update a dependency file regression-fix Pull request that fixes a regression in one of the previous Jenkins releases labels Jul 2, 2021
@basil basil requested a review from timja July 2, 2021 18:46
@MarkEWaite MarkEWaite added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jul 2, 2021
@MarkEWaite
Copy link
Contributor

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.

@jglick
Copy link
Member

jglick commented Jul 2, 2021

As a temporary hotfix, OK.

@basil basil added on-hold This pull request depends on another event/release, and it cannot be merged right now and removed ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback labels Jul 2, 2021
@basil
Copy link
Member Author

basil commented Jul 2, 2021

Putting this on hold, since in jenkinsci/jackson2-api-plugin#82 I figured out the root cause (a design problem in Jackson/StAX/Woodstox) and a way to work around the problem at each call site, implemented for windows-azure-storage in jenkinsci/azure-storage-plugin#195. Based on the discussion in jenkinsci/jackson2-api-plugin#82, it seems that Jesse's preference is to patch each call site to work around the problem rather than to keep wstx-asl in the WAR. Implicit in this preference is the notion that there won't be too many of these so as to cause serious disruption, which I cannot prove one way or another with any data. Right now, we are only aware of one such call site, but others may be discovered in the future. Let's agree on a tentative plan to patch each call site as in jenkinsci/azure-storage-plugin#195 as problems are discovered. Hopefully there won't be too many of these. If there are, then we can always revisit this PR.

@timja
Copy link
Member

timja commented Jul 3, 2021

It may still be worth hot-fixing it with this patch, the above work-around doesn't seem to work fully, and there's at least 2 plugins affected.

I've raised an upstream patch to jackson which may make this better in the future

@basil
Copy link
Member Author

basil commented Jul 3, 2021

It may still be worth hot-fixing it with this patch

I leave the decision to the members of the Jenkins core team, including you and Jesse. My preference, as stated in this PR description and in jenkinsci/jackson2-api-plugin#82, is to restore functionality for end users ASAP by re-adding Woodstox to Jenkins core (as proposed in this PR) and to try the removal again later on when we are more confident that it won't cause problems.

@jglick
Copy link
Member

jglick commented Jul 3, 2021

restore functionality for end users ASAP by re-adding Woodstox to Jenkins core (as proposed in this PR) and to try the removal again later on when we are more confident that it won't cause problems

This seems reasonable.

@timja timja added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed on-hold This pull request depends on another event/release, and it cannot be merged right now labels Jul 4, 2021
@timja
Copy link
Member

timja commented Jul 4, 2021

There's now a fix merged into Jackson upstream which has been verified to fix this issue.
FasterXML/jackson-dataformat-xml#482

Timeline for jackson 2.12.4 is approximately a month.

@basil did raise a workaround in the plugin but there's more patching needed, and a risk we will be miss places.

I think we can safely re-add this to the war and remove after:

  • Jackson is released
  • jackson2-api-plugin released
  • azure-sdk-plugin updated to remove jackson-databind-xml from it's package (required jackson2-api-plugin release)
  • jenkinsci/bom updated for new jackson (ideally)
  • azure-sdk-plugin, azure-storage-plugin, azure-artifact-manager-plugin updated to depend on required jackson version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback regression-fix Pull request that fixes a regression in one of the previous Jenkins releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants