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

Don't let getSession cause an infinite loop if it throws IllegalStateException #216

Merged
merged 1 commit into from
Dec 3, 2020
Merged

Don't let getSession cause an infinite loop if it throws IllegalStateException #216

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/main/ruby/jruby/rack/session_store.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,12 @@ def get_servlet_session(env, create = false)
unless servlet_request = env['java.servlet_request']
raise "JavaServletStore expects a servlet request at env['java.servlet_request']"
end
servlet_session = servlet_request.getSession(create)
servlet_session =
Copy link

@betesh betesh Dec 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This assignment should be outside the rescue on line 102. Otherwise, while it avoids the infinite loop, it makes the exception harder to track down because it's now wrapped in another exception.

Error tracking tools such as New Relic and Honeybadger would only display the stacktrace of the Ruby RuntimeError here, whereas moving this to after line 104 would make it possible to avoid wrapping this so that the stacktrace of the original IllegalStateException would be displayed.

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'm not sure if it is safe to move the assignment outside the rescue. The variable is used on line 89, 90, and 100, so moving it might break the intent of the code and the functionality of the retry. The comments of the rescue/retry makes it sound like it is for servlet_session.getCreationTime, so maybe refactoring the code to not need the retry would be best, but I'd rather someone who knows the intent of the code look at it and possibly offer their take on what should happen.

I could potentially have it raise a Java exception instead which chains the original exception, however I'm not entirely sure how useful the original stack trace will be... the source of the exception will be the servlet_request.getSession(create) call, which is the only line within the begin/rescue I added. This will trace to the Tomcat code that originated it, but I don't think there's much value in that stack trace, unless you want to dig into the Tomcat code to investigate further, but I think the message is much more valuable. In our specific case that triggered the infinite loop, the message indicated we had already committed the response, so you can't create a session at that point, so the stack trace wouldn't help us, just the message.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Mike, the outer rescue IllegalStateException was intended to also cover the getSession piece.
otherwise it would make sense to minimize the (original) outer rescue only to handle getCreationTime.
the real question would be whether servlet engines such as Tomcat also throw the error on recoverable causes ...

begin
servlet_request.getSession(create)
rescue java.lang.IllegalStateException => e
raise "Failed to obtain session due to IllegalStateException: #{e.message}"
end
env[ENV_SERVLET_SESSION_KEY] = servlet_session
end
rescue java.lang.IllegalStateException # cached session invalidated
Expand Down