[JENKINS-60867] Prevent creation of duplicated SetupWizard singleton instances on Jenkins startup

[Wadeck]

Follow-up of https://issues.jenkins-ci.org/browse/JENKINS-59017.

More details in the ticket: JENKINS-60867.

Proposed changelog entries

• Prevent creation of duplicated SetupWizard singleton instances on Jenkins startup

Proposed upgrade guidelines

N/A

Submitter checklist

• 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
• [n/a] Appropriate autotests or explanation to why this change has no tests
• [n/a] For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@BogdanSukonnov as he proposed the symptom correction
@Vlatombe as one of our SetupWizard / init reactor

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 should exist and be labeled as lts-candidate

[BogdanSukonnov]

Hi @Wadeck ! Thank you for including me in a reviewers! Your solution is so elegant! I just want to be sure that it works fine. You didn't specify in PR description that you actually tested Jenkins with this changes. Is setup wizard still works?

[Wadeck]

Is setup wizard still works?

I tested manually the two scenarios explained in the Jira ticket. One with a SetupWizard being processed to the end and one with a SetupWizard being interrupted and restarting to finish it. In both situation the wizard was fine (as before). :)

[BogdanSukonnov]

LGTM

[BogdanSukonnov]

LGTM

[BogdanSukonnov]

Great! I approved this PR, thanks! I was wondering how else could we break Setup Wizard. And it appears that it is possible. Try to add this test to SetupWizardRestartTest https://gist.github.com/BogdanSukonnov/f526d9bf3db5fa25fbb57928e1e3728b

[BogdanSukonnov]

What do you think about other approach? We can add hashCode() and equals() implementation to the anonimous class private final Filter FORCE_SETUP_WIZARD_FILTER = new Filter() { in the SetupWizard class, so that every instance of this filter will be equal.
Thus PluginServletFilter.hasFilter(FORCE_SETUP_WIZARD_FILTER) and
PluginServletFilter.removeFilter(FORCE_SETUP_WIZARD_FILTER) should work fine.
And, maybe, it will be better to replace getInstance(j.servletContext).list.remove(filter); in hudson.util.PluginServletFilter with some implementation, that removes not only first occurrence, but all occurrences - it will be head-shot :)

[Wadeck]

@BogdanSukonnov Thanks for your feedbacks!

1. I think the change on the "new" is the best way to solve the problem as it was the root cause of the problem. Normally in Jenkins environment, we do not expect to have multiple instance of an Extension. Think of it like in Spring about the bean, we do not expect developer to create the instance themself, but just retrieve the existing one, created and managed by Spring directly.
2. hashCode + equals or putting the constant as a static instance could have also solved the problem, but from my PoV, just hidden the real cause of the issue.

What you did initially with the additional condition inside the filter was a good solution to remove the symptom of the bug, in this approach I am trying to ensure the root cause is cured. All the proposals should work as well, I just "feel" that this way is the "cleanest".

[BogdanSukonnov]

@Wadeck thanks for clarifying.

[res0nance]

IIUC this corrects an older fix(the issue is still fixed and behaviour is identical to the user)

[oleg-nenashev]

Do we really want to skip the changelog here?

[timja]

I don’t think so

[res0nance]

Maybe not my initial thought was around behaviour being identical so users do not need a notification via changelog. Although an internal changelog would be fine as well.

[oleg-nenashev]

I plan to merge it tomorrow if no negative feedback

[oleg-nenashev]

I reclassified it as a bug, because singleton duplication might be a root cause of other Setup Wizard issues (reported or future ones). Thanks to @Wadeck for catching it!