FISH-766 Improper session synchronization of session map #4479

sgflt · 2020-02-09T12:11:30Z

Description

This is a bug fix of #4280

Important Info

Dependant PRs

Blockers

Testing

New tests

none

Testing Performed

none

Test suites executed

none

Testing Environment

Documentation

Notes for Reviewers

Reviewer should check the synchronization was incorrect (asymetric) and now each write attempt is locked to prevent readers to receive ConcurrentModificationException.

- cache could be read by another thread and put caused ConcurrentModificationException

- synchronization object should be final, that is what Sonar said

rdebusscher · 2020-04-03T07:59:45Z

@sgflt We are awaiting the signed CLA (https://github.com/payara/Payara/blob/master/PayaraCLA.pdf) otherwise we need to close this PR.

smillidge · 2020-04-06T13:19:11Z

We've got the CLA

lprimak

Instead of synchronized keyword around HashMap,
why don't you replace it with ConcurrentHashMap instead?

sgflt · 2020-06-24T05:50:16Z

Just for minimal change. But why not. I can try to use ConcurrentHashMap.

…er-session-synchronization

sgflt · 2020-07-19T14:05:56Z

WIP: Replacing synchronized by ConcurrentHashMap is not so trivial. Some parts of code have different semantics that may break session management. Anyways I am keeping to dig deeper.

sgflt · 2020-07-19T14:09:37Z

What does S1AS8 6155481 START kind of comments mean? Are they useful or just garbage?

- as found before isValid has expiration side effect - checkValid flag is meant to be short circuit

lprimak · 2020-07-19T17:09:27Z

I think that's remanents of Sun / Oracle issue tracking before JIRA -type or source control integration

lprimak · 2020-07-19T17:10:20Z

WIP: Replacing synchronized by ConcurrentHashMap is not so trivial. Some parts of code have different semantics that may break session management. Anyways I am keeping to dig deeper.

why not? I don't see any difference with that and synchronize, except synchronize has to be used in all parts of the code (even future parts) but not ConcurrentHashMap

sgflt · 2020-07-19T17:19:16Z

appserver/web/web-glue/src/main/java/com/sun/enterprise/security/web/GlassFishSingleSignOn.java

-                    }
+            this.cache.forEach((ssoId, sso) -> {
+                if (sso.isEmpty() && sso.getLastAccessTime() < tooOld) {
+                    removals.add(ssoId);
                }


There is a little change in behavior.

Old implementation locked SSOs and created a list of expired SSOs. No one could create a new SSO.

New implementation uses what is available and does not block creation of a new SSO (which is not taken for expiration in given round). However probability of a new SSO being expired is so small so I think this change should be OK for a real world usage.

- no null is allowed, only empty collection

sgflt · 2020-07-19T18:32:32Z

appserver/web/web-core/src/main/java/org/apache/catalina/session/StandardManager.java

@@ -985,16 +982,14 @@ public void processExpires() {

        long timeNow = System.currentTimeMillis();

-        Session[] sessions = findSessions();
-        if (sessions != null) {


nullcheck for broken contract removed

sgflt · 2020-07-19T18:32:45Z

appserver/web/web-core/src/main/java/org/apache/catalina/session/StandardManager.java

@@ -882,21 +881,19 @@ public void stop(boolean isShutdown) throws LifecycleException {
        }

        // Expire all active sessions and notify their listeners
-        Session sessions[] = findSessions();
-        if (sessions != null) {


nullcheck for broken contract removed

sgflt · 2020-07-19T18:33:06Z

appserver/web/web-core/src/main/java/org/apache/catalina/session/CookiePersistentManager.java

@@ -111,8 +111,8 @@ public void clearSessions() {
    }

    @Override
-    public Session[] findSessions() {
-        return null;


broken contract fixed

dmatej · 2020-07-20T13:44:47Z

What does S1AS8 6155481 START kind of comments mean? Are they useful or just garbage?

S1AS8 means SunOne Application Server 8 which used same system as the Sun JDK, now dead.

MeroRai · 2020-11-17T13:33:11Z

jenkins test please

…-session-synchronization" This reverts commit 0892dff, reversing changes made to 39d14da.

…-synchronization FISH-766 Improper session synchronization of session map

sgflt added 2 commits February 9, 2020 13:05

payara#4280 Fixed synchronization issue

5d6e7cd

- cache could be read by another thread and put caused ConcurrentModificationException

payara#4281 Added final modifier to field

3215ce9

- synchronization object should be final, that is what Sonar said

rdebusscher added the PR: Awaiting CLA Contributor does not have a CLA or has submitted an unconfirmed CLA. label Feb 11, 2020

rdebusscher added the Status: Abandoned User has not supplied reproducers for bug report, soon to be closed if user doesn’t come back label Apr 3, 2020

smillidge added PR: CLA CLA submitted on PR by the contributor and removed PR: Awaiting CLA Contributor does not have a CLA or has submitted an unconfirmed CLA. labels Apr 6, 2020

rdebusscher added community-contribution and removed Status: Abandoned User has not supplied reproducers for bug report, soon to be closed if user doesn’t come back labels Apr 6, 2020

lprimak suggested changes Jun 23, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into issue-4280-improp…

6c64922

…er-session-synchronization

sgflt added 3 commits July 19, 2020 16:36

payara#4280 Replaced HashMap by ConcurrentHashMap in SingleSignOn

b298f29

payara#4280 clean up code in GlassFishSingleSignOn

a00f866

payara#4280 Fixed unwanted double expiration

1754479

- as found before isValid has expiration side effect - checkValid flag is meant to be short circuit

sgflt commented Jul 19, 2020

View reviewed changes

sgflt marked this pull request as draft July 19, 2020 17:47

sgflt added 2 commits July 19, 2020 20:04

payara#4280 Deleted unused variable

dc43453

payara#4280 Fixed contract violation of findSessions

bef9a82

- no null is allowed, only empty collection

sgflt commented Jul 19, 2020

View reviewed changes

sgflt marked this pull request as ready for review July 21, 2020 05:38

lprimak approved these changes Jul 24, 2020

View reviewed changes

MeroRai changed the title ~~Fixes #4280 improper session synchronization~~ FISH-766 Improper session synchronization of session map Nov 18, 2020

MeroRai merged commit 0892dff into payara:master Nov 18, 2020

cubastanley added a commit to cubastanley/Payara that referenced this pull request Dec 4, 2020

Revert "Merge pull request payara#4479 from sgflt/issue-4280-improper…

7b218e2

…-session-synchronization" This reverts commit 0892dff, reversing changes made to 39d14da.

Cousjava pushed a commit to Cousjava/Payara that referenced this pull request Jan 27, 2021

Merge pull request payara#4479 from sgflt/issue-4280-improper-session…

f839d58

…-synchronization FISH-766 Improper session synchronization of session map

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FISH-766 Improper session synchronization of session map #4479

FISH-766 Improper session synchronization of session map #4479

sgflt commented Feb 9, 2020

rdebusscher commented Apr 3, 2020

smillidge commented Apr 6, 2020

lprimak left a comment

sgflt commented Jun 24, 2020

sgflt commented Jul 19, 2020

sgflt commented Jul 19, 2020

lprimak commented Jul 19, 2020

lprimak commented Jul 19, 2020

sgflt Jul 19, 2020 •

edited

Loading

sgflt Jul 19, 2020

sgflt Jul 19, 2020

sgflt Jul 19, 2020 •

edited

Loading

dmatej commented Jul 20, 2020

MeroRai commented Nov 17, 2020

FISH-766 Improper session synchronization of session map #4479

FISH-766 Improper session synchronization of session map #4479

Conversation

sgflt commented Feb 9, 2020

Description

Important Info

Dependant PRs

Blockers

Testing

New tests

Testing Performed

Test suites executed

Testing Environment

Documentation

Notes for Reviewers

rdebusscher commented Apr 3, 2020

smillidge commented Apr 6, 2020

lprimak left a comment

Choose a reason for hiding this comment

sgflt commented Jun 24, 2020

sgflt commented Jul 19, 2020

sgflt commented Jul 19, 2020

lprimak commented Jul 19, 2020

lprimak commented Jul 19, 2020

sgflt Jul 19, 2020 • edited Loading

Choose a reason for hiding this comment

sgflt Jul 19, 2020

Choose a reason for hiding this comment

sgflt Jul 19, 2020

Choose a reason for hiding this comment

sgflt Jul 19, 2020 • edited Loading

Choose a reason for hiding this comment

dmatej commented Jul 20, 2020

MeroRai commented Nov 17, 2020

sgflt Jul 19, 2020 •

edited

Loading

sgflt Jul 19, 2020 •

edited

Loading