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.

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

Fish 8174 improve thread expire validation to read lastaccesstime #6637

Conversation

breakponchito
Copy link
Contributor

@breakponchito breakponchito commented Apr 4, 2024

Adding fix to guarantee each thread update lastaccesstime and thisAccessedTime after validating if session hasExpired.

Description

This is a fix for a reported bug regarding http session management when using cluster configuration with lite and normal Payara micro instances.

Important Info

Blockers

Testing

New tests

Testing Performed

Manual testing with reproducers included on the ticket:
https://payara.atlassian.net/browse/FISH-8174
not only can be tested with docker configuration but also with local environment with stand alone Payara Micro. Here the steps:
Use following war file, and configuration files on the following zip:
Reproducer.zip

unzip on the place where the payara micro jar is installed:

image

the application on the war file set the session timeout limit to 1 minute, if you need to configure more time use the following zip file with the code and customize the time as you need:
Shared-Session-Code.zip

then create two instances of Payara Micro with the following commands:
instance1:
java -jar payara-micro.jar --clustermode multicast --clusterName myService-cluster --deploy Shared-Session-1.0.war --nohostaware --logProperties logging.properties --hzConfigFile payara-hazelcast.xml

image

instance2:
java -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=9002 -jar payara-micro.jar --lite --clustermode multicast --clusterName myService-cluster --deploy Shared-Session-1.0.war --nohostaware --logProperties logging.properties --hzConfigFile payara-hazelcast.xml --port 8081

image

open browser with two tabs for the following URLs:
open first and with a chronometer, from the mobile calculate 1 minute
http://localhost:8080/Shared-Session-1.0/safe
this is going to create the session

then open second with the other url:
before to achieve the timeout (60 seconds), for example second 57 or 58
http://localhost:8081/Shared-Session-1.0/safe

then you will see that the session is not recreated, just the values for the lastaccesstime and the thisAccessedTime are updated this will fix the problem of the gap for the session. From the reported issue the session is expired and created a new one 2 or 3 seconds before the timeout
image

image

for 2 minutes:
image
image
image

I tested the following configuration of timeouts:

1 minute
2 minutes

then I tested with 3 instances, trying to call each instance 2 or 3 seconds before timeout

and last with 4 instances, trying to call each instance 2 or 4 seconds before timeout

Testing Environment

Windows 11, Azul JDK 11, Maven 3.9.5, Payara 6
Ubuntu 20.04, jdk 11, Maven 3.8.6, Payara 6

Documentation

Notes for Reviewers

@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito breakponchito requested a review from Pandrex247 April 4, 2024 19:13
Copy link
Member

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

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

A number of files need their copyright updating, and as you've mentioned yourself the logging needs cleaning up (at the very least setting the log level to finest!), and I don't have time to properly test it right now, but the overall concept LGTM.

@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito
Copy link
Contributor Author

Jenkins test please

…-thread-expire-validation-to-read-lastaccesstime
@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito
Copy link
Contributor Author

Jenkins test please

2 similar comments
@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito
Copy link
Contributor Author

Jenkins test please

Copy link
Contributor

@luiseufrasio luiseufrasio left a comment

Choose a reason for hiding this comment

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

LGTM!

@breakponchito
Copy link
Contributor Author

Jenkins test please

1 similar comment
@breakponchito
Copy link
Contributor Author

Jenkins test please

@breakponchito breakponchito merged commit 294ec57 into payara:master May 24, 2024
1 check passed
@lprimak
Copy link
Contributor

lprimak commented Jun 5, 2024

Unfortunately this PR introduced a regression.
As you can see below, there are dangling BackgroundSessionProcessor threads.
In other words, more BackgroundSessionProcessor threads than the BackgroundProcessor threads after an webapp is undeployed.

Screen Shot 2024-06-04 at 5 59 14 PM

Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Jun 19, 2024
…hread-expire-validation-to-read-lastaccesstime

Fish 8174 improve thread expire validation to read lastaccesstime
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Jun 26, 2024
…hread-expire-validation-to-read-lastaccesstime

Fish 8174 improve thread expire validation to read lastaccesstime
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Jul 2, 2024
…hread-expire-validation-to-read-lastaccesstime

Fish 8174 improve thread expire validation to read lastaccesstime
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Jul 5, 2024
…hread-expire-validation-to-read-lastaccesstime

Fish 8174 improve thread expire validation to read lastaccesstime
@lprimak
Copy link
Contributor

lprimak commented Sep 18, 2024

Another regression introduced by this PR found. Stay tuned, I submitted another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants