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.

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

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

Merged
merged 1 commit into from Jul 11, 2019
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion core/src/main/java/hudson/security/HttpSessionContextIntegrationFilter2.java
View file
Open in desktop
Expand Up @@ -84,7 +84,13 @@ private boolean hasInvalidSessionSeed(Authentication authentication, HttpSession
return false;
}

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

Choose a reason for hiding this comment

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

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

logger.warn("Encountered IllegalStateException trying to get a user. System init may not have completed yet. Invalidating user session.");
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

@proski proski Jul 10, 2019 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@jvz do you consider this comment as resolved?

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, good with me.

return false;
}
if (userFromSession == null) {
// no requirement for further test as there is no user inside
return false;
Expand Down