[JENKINS-55945] IllegalStateException trying to get user. #4107

jeffret-b · 2019-07-09T20:18:42Z

This change catches the exception and returns false, invalidating the user session. This behavior is sensible, because if we are unable to validate the user we must assume the session is invalid.
Catching the error at this point reportedly helps with some scenarios and shouldn't cause any further harm.

This error shows up only in some scenarios or environments and I have been unable to reproduce it myself despite hours spent trying. This change is based upon one proposed and tested on the Jira ticket by Pavel Roskin. Without the change, Pavel reports encountering the error reliably. With the change, it didn't manifest.

There is a deeper problem here, as the exception is correctly thrown because of an illegal state. There must be one instance of AllUsers once the init sequence is complete and Jenkins shouldn't respond if it isn't. I encountered a similar error once, but in that case it originated in Jenkins.get(). It would be nice to figure out what to do about the illegal state, but I haven't been able to dig up any clues. I figure that this minor change will at least address the situation seen by some users.

As I haven't been able to reproduce the problem or determine any cause, I haven't been able to readily provide any autotests for the illegal state. I could try inserting some mocks, but those are notoriously clumsy and difficult, particularly for these sorts of things.

See JENKINS-55945.

Proposed changelog entries

Catch an occasional IllegalStateException on Jenkins restart and invalidate the user session. (JENKINS-55945)

Submitter checklist

JIRA issue is well described
Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
* Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
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

This change catches the exception and returns false, invalidating the user session. This behavior is sensible, because if we are unable to validate the user we must assume the session is invalid. Catching the error at this point reportedly helps with some scenarios and shouldn't cause any further harm.

proski · 2019-07-09T22:49:09Z

core/src/main/java/hudson/security/HttpSessionContextIntegrationFilter2.java

+        User userFromSession;
+        try {
+            userFromSession = User.getById(authentication.getName(), false);
+        } catch (IllegalStateException ise) {


No-idiomatic abbreviations set a bad example to others. Common names for an exception are e, x and ex. Not a blocker.

proski · 2019-07-09T22:51:16Z

Sorry for not submitting this PR on my own! Looks good to me.

oleg-nenashev

Why not just every RuntimeException ?

jeffret-b · 2019-07-10T15:51:54Z

Sorry for not submitting this PR on my own! Looks good to me.

I figured I'd go ahead and get it proposed. I don't want to take away from the work you've done on it, though. You've been able to verify it fixes the issue, whereas no matter what I've tried I can't reproduce it.

jeffret-b · 2019-07-10T15:55:19Z

Why not just every RuntimeException ?

We could, but I wanted to keep this narrowly focused on the specific issue some people have reported. I'm inclined to keep it that way, especially considering the reproduction difficulties and the init sequencing issues that must exist.

oleg-nenashev

whatever works

oleg-nenashev · 2019-07-10T18:15:30Z

Will merge tomorrow if no negative feedback

jvz · 2019-07-10T18:36:01Z

core/src/main/java/hudson/security/HttpSessionContextIntegrationFilter2.java

+        try {
+            userFromSession = User.getById(authentication.getName(), false);
+        } catch (IllegalStateException ise) {
+            logger.warn("Encountered IllegalStateException trying to get a user. System init may not have completed yet. Invalidating user session.");


Would it be useful to include the exception in the log message?

I debated that myself. My opinion is that because the handling is so narrow, it would just introduce clutter to include the exception. If we broadened the catch, then we would definitely want to add the exception.

It the issue is understood, a stack trace only makes the log look scarier. It's easy to misread a stack trace as an exception that wasn't handled properly.

On the other hand, the message from the exception could be useful to make sure we caught the correct exception.

@jvz do you consider this comment as resolved?

Oh yeah, good with me.

This change catches the exception and returns false, invalidating the user session. This behavior is sensible, because if we are unable to validate the user we must assume the session is invalid. Catching the error at this point reportedly helps with some scenarios and shouldn't cause any further harm. (cherry picked from commit 8349ceb)

During my testing I have noticed that from time to time when restarting Jenkins, an `IllegalStateException` is thrown. This issue is similar to jenkinsci/jenkins#4107 in fact it is the same issue as we make the same call in our code: https://github.com/jenkinsci/crowd2-plugin/blob/ff4affe98a969c5bcd4de4211656a8db57e841b4/src/main/java/de/theit/jenkins/crowd/CrowdAuthenticationToken.java#L125 Before this patch the log looked like this: ``` Apr 24 08:23:18 Jenkins-McdCore java[516]: INFO: Jenkins stopped Apr 24 08:23:19 Jenkins-McdCore java[516]: Apr 24, 2023 8:23:19 AM hudson.init.impl.InstallUncaughtExceptionHandler handleException Apr 24 08:23:19 Jenkins-McdCore java[516]: WARNING: Caught unhandled exception with ID 2841997a-ee9d-4e6e-a1d0-e36079f288ec Apr 24 08:23:19 Jenkins-McdCore java[516]: java.lang.IllegalStateException: Expected 1 instance of hudson.model.User$AllUsers but got 0 Apr 24 08:23:19 Jenkins-McdCore java[516]: at hudson.ExtensionList.lookupSingleton(ExtensionList.java:454) Apr 24 08:23:19 Jenkins-McdCore java[516]: at hudson.model.User$AllUsers.getInstance(User.java:1098) Apr 24 08:23:19 Jenkins-McdCore java[516]: at hudson.model.User$AllUsers.get(User.java:1116) Apr 24 08:23:19 Jenkins-McdCore java[516]: at hudson.model.User.getOrCreateById(User.java:535) Apr 24 08:23:19 Jenkins-McdCore java[516]: at hudson.model.User.getById(User.java:638) Apr 24 08:23:19 Jenkins-McdCore java[516]: at de.theit.jenkins.crowd.CrowdAuthenticationToken.updateUserInfo(CrowdAuthenticationTok> Apr 24 08:23:19 Jenkins-McdCore java[516]: at de.theit.jenkins.crowd.CrowdRememberMeServices.autoLogin(CrowdRememberMeServices.java> Apr 24 08:23:19 Jenkins-McdCore java[516]: at de.theit.jenkins.crowd.CrowdServletFilter.doFilter(CrowdServletFilter.java:177) ``` The server would still restart but would produce `Oops! A problem occurred while processing the request.`. After the patch: ``` Apr 24 20:13:22 Jenkins-CrowdAAD java[242796]: Apr 24, 2023 8:13:22 PM hudson.lifecycle.Lifecycle onStatusUpdate Apr 24 20:13:22 Jenkins-CrowdAAD java[242796]: INFO: Jenkins stopped Apr 24 20:13:23 Jenkins-CrowdAAD java[242796]: Apr 24, 2023 8:13:23 PM de.theit.jenkins.crowd.CrowdRememberMeServices autoLogin Apr 24 20:13:23 Jenkins-CrowdAAD java[242796]: WARNING: Encountered IllegalStateException. System init may not have completed yet. Apr 24 20:13:23 Jenkins-CrowdAAD java[242796]: Apr 24, 2023 8:13:23 PM hudson.security.HttpSessionContextIntegrationFilter2 hasInvalidSessionSeed Apr 24 20:13:23 Jenkins-CrowdAAD java[242796]: WARNING: Encountered IllegalStateException trying to get a user. System init may not have completed yet. Invalidating user session. Apr 24 20:13:25 Jenkins-CrowdAAD java[242796]: Running from: /opt/jenkins_home/jenkins.war Apr 24 20:13:25 Jenkins-CrowdAAD java[242796]: webroot: /opt/jenkins_home/war Apr 24 20:13:25 Jenkins-CrowdAAD java[242796]: Apr 24, 2023 8:13:25 PM winstone.Logger logInternal Apr 24 20:13:25 Jenkins-CrowdAAD java[242796]: INFO: Beginning extraction from war file ```

jeffret-b requested review from Wadeck and jvz July 9, 2019 20:18

proski approved these changes Jul 9, 2019

View reviewed changes

oleg-nenashev reviewed Jul 10, 2019

View reviewed changes

oleg-nenashev approved these changes Jul 10, 2019

View reviewed changes

oleg-nenashev 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 10, 2019

jvz reviewed Jul 10, 2019

View reviewed changes

jvz approved these changes Jul 11, 2019

View reviewed changes

oleg-nenashev merged commit 8349ceb into jenkinsci:master Jul 11, 2019

akouznetchik mentioned this pull request Apr 26, 2023

Catch IllegalStateException jenkinsci/crowd2-plugin#179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-55945] IllegalStateException trying to get user. #4107

[JENKINS-55945] IllegalStateException trying to get user. #4107

jeffret-b commented Jul 9, 2019

proski Jul 9, 2019

proski commented Jul 9, 2019

oleg-nenashev left a comment

jeffret-b commented Jul 10, 2019

jeffret-b commented Jul 10, 2019

oleg-nenashev left a comment

oleg-nenashev commented Jul 10, 2019

jvz Jul 10, 2019

jeffret-b Jul 10, 2019

proski Jul 10, 2019 •

edited

oleg-nenashev Jul 11, 2019

jvz Jul 11, 2019

[JENKINS-55945] IllegalStateException trying to get user. #4107

[JENKINS-55945] IllegalStateException trying to get user. #4107

Conversation

jeffret-b commented Jul 9, 2019

Proposed changelog entries

Submitter checklist

proski Jul 9, 2019

Choose a reason for hiding this comment

proski commented Jul 9, 2019

oleg-nenashev left a comment

Choose a reason for hiding this comment

jeffret-b commented Jul 10, 2019

jeffret-b commented Jul 10, 2019

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Jul 10, 2019

jvz Jul 10, 2019

Choose a reason for hiding this comment

jeffret-b Jul 10, 2019

Choose a reason for hiding this comment

proski Jul 10, 2019 • edited

Choose a reason for hiding this comment

oleg-nenashev Jul 11, 2019

Choose a reason for hiding this comment

jvz Jul 11, 2019

Choose a reason for hiding this comment

proski Jul 10, 2019 •

edited