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

[smellsblue]

This fixes #215

I have implemented this as a monkeypatch to our system and verified the infinite loop goes away for us, and figured it would be useful to submit this upstream.

I break the infinite loop by catching the IllegalStateException when caused by getSession(boolean) and throw it as a RuntimeError to avoid triggering the loop.

This might be better solved by restructuring the code, or changing what the begin/rescue is wrapping, or preventing infinite retries, but I patched this in our system like this to avoid making too much changes to the original code and possibly introducing a different bug.

Please let me know if you would like me to attempt a different approach.

[betesh]

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.

[smellsblue]

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.

[kares]

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 ...