You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openwebbeans.apache.org by GitBox <gi...@apache.org> on 2021/05/13 13:51:52 UTC
[GitHub] [openwebbeans] stephenc opened a new pull request #34: Workaround Jetty ISE for invalidating sessions
stephenc opened a new pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34
https://github.com/eclipse/jetty.project/blob/4204526d2fdad355e233f6bf18a44bfe028ee00b/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java#L467-L478
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] rmannibucau commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r632489637
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
@tandraschko I'd prefer to have a custom extended listener or service in jetty module with a property (from openwebbeans.properties/OpenWebBeansConfiguration) to enable/disable it by conf. In main code (generic web module) it can hurt all other containers since it is about hiding a bug and not swallowing an expected exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] rmannibucau commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r632500938
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
> @rmannibucau I was not clear, let me try again.
>
> If the session is being "invalidated" (aka then about to be invalid) then you are correct `getLastAccedTime()` should not throw an exception as you say.
> However the method that is being patched here is as follows:
>
> https://github.com/apache/openwebbeans/blob/e9332767af5abd81867b21b8a4d10718733fb188/webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java#L588-L603
>
> The javadoc for this method _clearly_ states (with some typos)
>
> > @return {@code true} if the sessino is currently expiring or has already expired
>
> thus - either this javaodoc comment is wrong - as it must never be called by a session that has **already** expired (aka has already been invalidated) .
> or
> the code is buggy as stated as if called with a session that has already expired, then it will never return `true` but is guaranteed by the specification to throw an `IllegalArgumentException` for every implementation that adheres to the specification.
nop, if the session expired we got the listener called and session context is no more available so in terms of codepath it shouldn't be possible (mainly because of the links I sent previously).
The two cases we want to cover are:
* destroying the session on timeout (listener case, no issue calling this method)
* destroying it after the request (end of service()) when invalidate() is called manually in the session during the request
In both cases the session is not yet invalidated so is fully functional in the listener.
@tandraschko @stephenc org.apache.webbeans.web.context.WebContextsService#destroySessionImmediately looks like a toggle waiting for its configuration to be wired ;). Happy if this one and/or friends are wired here or in jetty module (kind of follow strict servlet lifecycle vs default cdi lifecycle).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] stephenc commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
stephenc commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r631905399
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
Also affects jetty 9.x too.
The issue is not as bad for me as it just causes log spam because exceptions thrown by the listener are caught and logged by jetty... but I thought better to create a PR to at least hint... Ain't nobody got time for JIRA
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] rmannibucau commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r632689634
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
@jtnord https://github.com/apache/openwebbeans/pull/34#discussion_r632493311 i can agree the comment is wrongly phrased but it must be understood as "already invalidated at the end of the request", the related commit explains it well by referencing the two cases i mentionned in https://github.com/apache/openwebbeans/pull/34#discussion_r632500938. We can get it fixed - I'm more than happy to review such a PR - but it will not solve the underlying jetty spec bug.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] jtnord commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
jtnord commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r632471170
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
> spec this must not happen (servlet spec) and session must not be invalidated before the listener is called so getLastAccessedTime is a valid call.
But the javadoc on this very method is saying " @return {@code true} if the sessino is currently expiring or has already expired"
If this is to be expected to be called with a session that "has already expired" then the spec says that `session.getLastAccessedTime()` will always throw an exception.
So there is a bug here - either the javadoc comment for this method is wrong and should never be passed a session that *has* expired or....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] stephenc closed pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
stephenc closed pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@openwebbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] stephenc commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
stephenc commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r632492164
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
I can confirm that:
```java
public class JettyBugWorkaroundWebContextService extends WebContextsService {
public JettyBugWorkaroundWebContextService(WebBeansContext webBeansContext) {
super(webBeansContext);
}
@Override
protected boolean sessionIsExpiring(HttpSession session) {
try {
return super.sessionIsExpiring(session);
} catch (IllegalStateException e) {
if ("Session not valid".equals(e.getMessage())) {
return true;
}
throw e;
}
}
}
```
coupled with an openwebbeans.properties, e.g.
```
configuration.ordinal=15
org.apache.webbeans.spi.ContextsService=...JettyBugWorkaroundWebContextService
```
Works around the issue and would be trivial to add to the owb-jetty9 module
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] tandraschko commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
tandraschko commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r632491866
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
i wonder why we even call sessionIsExpiring in this case? we are inside the HttpSessionListener#sessionDestroyed event stack, there is no need to check this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] rmannibucau commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r632478148
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
@jtnord: https://javaee.github.io/javaee-spec/javadocs/javax/servlet/http/HttpSession.html#getLastAccessedTime--
> IllegalStateException - if this method is called on an invalidated session
and https://javaee.github.io/javaee-spec/javadocs/javax/servlet/http/HttpSessionListener.html#sessionDestroyed-javax.servlet.http.HttpSessionEvent-
> Receives notification that a session is about to be invalidated.
So it can't be invalidated there by spec.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] tandraschko commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
tandraschko commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r632486753
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
IMO we should add the try/catch as it doesnt hurt for now + create a ticket in jetty
so we can remove it in ~1 year or so
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] jtnord commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
jtnord commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r632493311
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
@rmannibucau I was not clear, let me try again.
If the session is being "invalidated" (aka then about to be invalid) then you are correct `getLastAccedTime()` should not throw an exception as you say.
However the method that is being patched here is as follows:
https://github.com/apache/openwebbeans/blob/e9332767af5abd81867b21b8a4d10718733fb188/webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java#L588-L603
The javadoc for this method *clearly* states (with some typos)
> @return {@code true} if the sessino is currently expiring or has already expired
thus - either this javaodoc comment is wrong - as it must never be called by a session that has **already** expired (aka has already been invalidated) .
or
the code is buggy as stated as if called with a session that has already expired, then it will never return `true` but is guaranteed by the specification to throw an `IllegalArgumentException` for every implementation that adheres to the specification.
or
I am completely missing something else
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] rmannibucau commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r632495176
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
> i wonder why we even call sessionIsExpiring in this case? we are inside the HttpSessionListener#sessionDestroyed event stack, there is no need to check this
Blaming the commit you can see the details but idea is to be able to let request listener override expiry handling when invalidation happens in a request.
Indeed we can track it better (without time related test) but this kind of handling is needed but it is transversal to this thread/issue and we shouldn't merge both topics IMHO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] jtnord commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
jtnord commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r632471170
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
> spec this must not happen (servlet spec) and session must not be invalidated before the listener is called so getLastAccessedTime is a valid call.
But the javadoc on this very method is saying " @return {@code true} if the sessino is currently expiring or has already expired"
If this is to be expected to be called with a session that has expired then the spec says that `session.getLastAccessedTime()` will always throw an exception.
So there is a bug here - either the javadoc comment for this method is wrong and should never be passed a session that *has* expired or....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] stephenc commented on pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
stephenc commented on pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#issuecomment-899366169
Fixed in Jetty
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@openwebbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] jtnord commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
jtnord commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r632493311
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
@rmannibucau I was not clear, let me try again.
If the session is being "invalidated" (aka then about to be invalid) then you are correct `getLastAccedTime()` should not throw an exception as you say.
However the method that is being patched here is as follows:
https://github.com/apache/openwebbeans/blob/e9332767af5abd81867b21b8a4d10718733fb188/webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java#L588-L603
The javadoc for this method *clearly* states (with some typos)
> @return {@code true} if the sessino is currently expiring or has already expired
thus - either this javaodoc comment is wrong - as it must never be called by a session that has **already** expired (aka has already been invalidated) .
or
the code is buggy as stated as if called with a session that has already expired, then it will never return `true` but is guaranteed by the specification to throw an `IllegalArgumentException` for every implementation that adheres to the specification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwebbeans] rmannibucau commented on a change in pull request #34: Workaround Jetty ISE for invalidating sessions
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #34:
URL: https://github.com/apache/openwebbeans/pull/34#discussion_r631830461
##########
File path: webbeans-web/src/main/java/org/apache/webbeans/web/context/WebContextsService.java
##########
@@ -593,9 +593,17 @@ protected boolean sessionIsExpiring(HttpSession session)
int maxInactiveInterval = session.getMaxInactiveInterval();
if (maxInactiveInterval > 0)
{
- long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
- if (inactiveSince >= maxInactiveInterval)
+ try
{
+ long inactiveSince = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - session.getLastAccessedTime());
+ if (inactiveSince >= maxInactiveInterval)
+ {
+ return true;
+ }
+ }
+ catch (IllegalStateException e)
+ {
+ // Jetty will throw an ISE if you attempt to query the last accessed time of a session that is being invalidated
return true;
Review comment:
Hi @stephenc , we discussed it with @tandraschko and I think if it is, it must be in jetty module and it should likely be sortable without such a hack by adjusting the position of the listeners. By spec this must not happen (servlet spec) and session must not be invalidated before the listener is called so getLastAccessedTime is a valid call.
My main concern is this workaround hides actual bugs and wrong setups which shouldn't be. If jetty bug is not quickly fix we can detect jetty version in jetty module and wrap the listener to catch it but probably not in web module.
wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org