Skip to content

Commit

Permalink
Revert "Merge pull request payara#4479 from sgflt/issue-4280-improper…
Browse files Browse the repository at this point in the history 
…-session-synchronization"

This reverts commit 0892dff, reversing
changes made to 39d14da.
  • Loading branch information
@cubastanley
cubastanley committed Dec 4, 2020
1 parent 24bb640 commit 7b218e2
Show file tree
Hide file tree
Showing 9 changed files with 242 additions and 190 deletions.
110 changes: 54 additions & 56 deletions appserver/web/web-core/src/main/java/org/apache/catalina/Manager.java
View file

Large diffs are not rendered by default.

33 changes: 18 additions & 15 deletions appserver/web/web-core/src/main/java/org/apache/catalina/authenticator/SingleSignOn.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,12 @@
*/
// Portions Copyright [2016-2019] [Payara Foundation and/or its affiliates]
package org.apache.catalina.authenticator;

import java.io.IOException;
import java.security.Principal;
import java.text.MessageFormat;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.HashMap;
import java.util.Map;
import java.util.logging.Level;

import javax.servlet.ServletException;
Expand Down Expand Up @@ -118,7 +119,7 @@ public class SingleSignOn extends ValveBase
* The cache of SingleSignOnEntry instances for authenticated Principals, keyed by the cookie value that is used to
* select them.
*/
protected final ConcurrentMap<String, SingleSignOnEntry> cache = new ConcurrentHashMap<>();
protected Map<String, SingleSignOnEntry> cache = new HashMap<String, SingleSignOnEntry>();

/**
* The lifecycle event support for this component.
Expand Down Expand Up @@ -433,7 +434,9 @@ protected void deregister(String ssoId, Session session) {

// see if we are the last session, if so blow away ssoId
if (sso.isEmpty()) {
this.cache.remove(ssoId);
synchronized (cache) {
cache.remove(ssoId);
}
}
}

Expand All @@ -446,21 +449,17 @@ protected void deregister(String ssoId, Session session) {
* @param username Username used to authenticate this user
* @param password Password used to authenticate this user
*/
protected void register(
final String ssoId,
final Principal principal,
final String authType,
final String username,
final char[] password,
final String realmName
) {
protected void register(String ssoId, Principal principal, String authType, String username, char[] password, String realmName) {

if (debug >= 1) {
String msg = MessageFormat.format(rb.getString(LogFacade.REGISTERING_SSO_INFO),
new Object[] { ssoId, principal.getName(), authType });
log(msg);
}
synchronized (cache) {
cache.put(ssoId, new SingleSignOnEntry(ssoId, 0L, principal, authType, username, realmName));
}

this.cache.put(ssoId, new SingleSignOnEntry(ssoId, 0L, principal, authType, username, realmName));
}

// ------------------------------------------------------ Protected Methods
Expand Down Expand Up @@ -502,8 +501,12 @@ protected void log(String message, Throwable t) {
*
* @param ssoId Single sign on identifier to look up
*/
protected SingleSignOnEntry lookup(final String ssoId) {
return this.cache.get(ssoId);
protected SingleSignOnEntry lookup(String ssoId) {

synchronized (cache) {
return cache.get(ssoId);
}

}

/**
Expand Down
5 changes: 3 additions & 2 deletions ...erver/web/web-core/src/main/java/org/apache/catalina/session/CookiePersistentManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ public Session findSession(String id, HttpServletRequest request) throws IOExcep
if (cookies == null) {
return null;
}
String value = null;
for (Cookie cookie : cookies) {
if (cookieName.equals(cookie.getName())) {
return parseSession(cookie.getValue(), request.getRequestedSessionId());
Expand All @@ -111,8 +112,8 @@ public void clearSessions() {
}

@Override
public List<Session> findSessions() {
return Collections.emptyList();
public Session[] findSessions() {
return null;
}

@Override
Expand Down
16 changes: 12 additions & 4 deletions appserver/web/web-core/src/main/java/org/apache/catalina/session/ManagerBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ public abstract class ManagerBase implements Manager {
* The set of currently active Sessions for this Manager, keyed by
* session identifier.
*/
protected final Map<String, Session> sessions = new ConcurrentHashMap<>();
protected Map<String, Session> sessions = new ConcurrentHashMap<String, Session>();

// Number of sessions created by this manager
protected int sessionCounter=0;
Expand Down Expand Up @@ -896,11 +896,19 @@ public void clearSessions() {
}


/**
* Return the set of active Sessions associated with this Manager.
* If this Manager has no active Sessions, a zero-length array is returned.
*/
@Override
public List<Session> findSessions() {
public Session[] findSessions() {
// take a snapshot
Collection<Session> sessionsValues = this.sessions.values();
return new ArrayList<>(sessionsValues);
Collection<Session> sessionsValues = sessions.values();
List<Session> list = new ArrayList<Session>(sessionsValues.size());
for (Session session : sessionsValues) {
list.add(session);
}
return list.toArray(new Session[list.size()]);
}


Expand Down
81 changes: 51 additions & 30 deletions appserver/web/web-core/src/main/java/org/apache/catalina/session/PersistentManagerBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -594,14 +594,21 @@ public void clearStore() {
* Hercules: modified method
*/
protected void processExpires() {

if (!started)
return;

final List<Session> sessions = findSessions();
for (final Session session1 : sessions) {
Session sessions[] = findSessions();

for (Session session1 : sessions) {
StandardSession session = (StandardSession) session1;
/* START CR 6363689
if (!session.isValid()) {
*/
// START CR 6363689
if(!session.getIsValid() || session.hasExpired()) {
if(session.lockBackground()) {
// END CR 6363689
if(session.lockBackground()) {
try {
session.expire();
} finally {
Expand Down Expand Up @@ -944,21 +951,20 @@ public void unload() {
if (store == null)
return;

final List<Session> sessions = findSessions();
if (sessions.isEmpty())
Session sessions[] = findSessions();
int n = sessions.length;
if (n == 0)
return;

if (log.isLoggable(Level.FINE)) {
log.log(Level.FINE, LogFacade.SAVING_PERSISTED_SESSION, sessions.size());
log.log(Level.FINE, LogFacade.SAVING_PERSISTED_SESSION, String.valueOf(n));
}

for (final Session session : sessions) {
for (int i = 0; i < n; i++)
try {
swapOut(session);
swapOut(sessions[i]);
} catch (IOException e) {
// This is logged in writeSession()
}
}

}

Expand Down Expand Up @@ -1255,12 +1261,11 @@ public void stop() throws LifecycleException {
unload();
} else {
// Expire all active sessions
final List<Session> sessions = findSessions();
for (final Session session1 : sessions) {
final StandardSession session = (StandardSession) session1;
if (!session.isValid()) {
Session sessions[] = findSessions();
for (Session session1 : sessions) {
StandardSession session = (StandardSession) session1;
if (!session.isValid())
continue;
}
session.expire();
}
}
Expand Down Expand Up @@ -1315,15 +1320,15 @@ protected void processMaxIdleSwaps() {
if (!isStarted() || maxIdleSwap < 0)
return;

final List<Session> sessions = findSessions();
final long timeNow = System.currentTimeMillis();
Session sessions[] = findSessions();
long timeNow = System.currentTimeMillis();

// Swap out all sessions idle longer than maxIdleSwap
// FIXME: What's preventing us from mangling a session during
// a request?
if (maxIdleSwap >= 0) {
for (final Session session1 : sessions) {
final StandardSession session = (StandardSession) session1;
for (Session session1 : sessions) {
StandardSession session = (StandardSession) session1;
if (!session.isValid())
continue;
int timeIdle = // Truncate, do not round up
Expand All @@ -1349,35 +1354,40 @@ protected void processMaxIdleSwaps() {
* Hercules: modified method
*/
protected void processMaxActiveSwaps() {

if (!isStarted() || getMaxActiveSessions() < 0)
return;

final List<Session> sessions = findSessions();
Session sessions[] = findSessions();

// FIXME: Smarter algorithm (LRU)
if (sessions.size() <= getMaxActiveSessions())
if (getMaxActiveSessions() >= sessions.length)
return;

if(log.isLoggable(Level.FINE)) {
log.log(Level.FINE, LogFacade.TOO_MANY_ACTIVE_SESSION, sessions.size());
log.log(Level.FINE, LogFacade.TOO_MANY_ACTIVE_SESSION, sessions.length);
}
int toswap = sessions.size() - getMaxActiveSessions();
int toswap = sessions.length - getMaxActiveSessions();
long timeNow = System.currentTimeMillis();

for (int i = 0; i < sessions.size() && toswap > 0; i++) {
for (int i = 0; i < sessions.length && toswap > 0; i++) {
int timeIdle = // Truncate, do not round up
(int) ((timeNow - sessions.get(i).getLastAccessedTime()) / 1000L);
(int) ((timeNow - sessions[i].getLastAccessedTime()) / 1000L);
if (timeIdle > minIdleSwap) {
StandardSession session = (StandardSession) sessions.get(i);
StandardSession session = (StandardSession) sessions[i];
//skip the session if it cannot be locked
if(session.lockBackground()) {
if(log.isLoggable(Level.FINE)) {
log.log(Level.FINE, LogFacade.SWAP_OUT_SESSION, new Object[] {session.getIdInternal(), timeIdle});
}
try {
swapOut(session);
} catch (Exception e) {
} catch (java.util.ConcurrentModificationException e1) {
// This is logged in writeSession()
} catch (IOException e) {
// This is logged in writeSession()
} catch (Exception e) {
// This is logged in writeSession()
} finally {
session.unlockBackground();
}
Expand All @@ -1398,8 +1408,8 @@ protected void processMaxIdleBackups() {
if (!isStarted() || maxIdleBackup < 0)
return;

final List<Session> sessions = findSessions();
final long timeNow = System.currentTimeMillis();
Session sessions[] = findSessions();
long timeNow = System.currentTimeMillis();

// Back up all sessions idle longer than maxIdleBackup
if (maxIdleBackup >= 0) {
Expand All @@ -1417,8 +1427,12 @@ protected void processMaxIdleBackups() {
}
try {
writeSession(session);
} catch (Exception e) {
} catch (java.util.ConcurrentModificationException e1) {
// This is logged in writeSession()
} catch (IOException e) {
// This is logged in writeSession()
} catch (Exception e) {
// This is logged in writeSession()
} finally {
session.unlockBackground();
}
Expand All @@ -1428,4 +1442,11 @@ protected void processMaxIdleBackups() {
}

}

public String getMonitorAttributeValues() {
//FIXME if desired for monitoring 'file'
return "";
}


}
57 changes: 31 additions & 26 deletions appserver/web/web-core/src/main/java/org/apache/catalina/session/StandardManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -669,23 +669,24 @@ public void writeSessions(OutputStream os, boolean doExpire)
}

// Write the number of active sessions, followed by the details
final StandardSession[] currentStandardSessions;
StandardSession[] currentStandardSessions = null;
synchronized (sessions) {
if (log.isLoggable(Level.FINE))
log.log(Level.FINE, "Unloading {0} sessions", sessions.size());
try {
// START SJSAS 6375689
for (final Session actSession : findSessions()) {
for (Session actSession : findSessions()) {
StandardSession session = (StandardSession) actSession;
session.passivate();
}
// END SJSAS 6375689
final List<Session> currentSessions = findSessions();
int size = currentSessions.size();
Session[] currentSessions = findSessions();
int size = currentSessions.length;
currentStandardSessions = new StandardSession[size];
oos.writeObject(size);
for (int i = 0; i < size; i++) {
final StandardSession session = (StandardSession) currentSessions.get(i);
StandardSession session =
(StandardSession) currentSessions[i];
currentStandardSessions[i] = session;
/* SJSAS 6375689
session.passivate();
Expand Down Expand Up @@ -881,19 +882,21 @@ public void stop(boolean isShutdown) throws LifecycleException {
}

// Expire all active sessions and notify their listeners
final List<Session> sessions = findSessions();
for (final Session session : sessions) {
if (!session.isValid()) {
continue;
}
try {
session.expire();
} catch (Throwable t) {
// Ignore
} finally {
// Measure against memory leaking if references to the session
// object are kept in a shared field somewhere
session.recycle();
Session sessions[] = findSessions();
if (sessions != null) {
for (Session session : sessions) {
if (!session.isValid()) {
continue;
}
try {
session.expire();
} catch (Throwable t) {
// Ignore
} finally {
// Measure against memory leaking if references to the session
// object are kept in a shared field somewhere
session.recycle();
}
}
}

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

long timeNow = System.currentTimeMillis();

final List<Session> sessions = findSessions();
for (final Session session : sessions) {
final StandardSession sess = (StandardSession) session;
if (sess.lockBackground()) {
try {
sess.isValid();
} finally {
sess.unlockBackground();
Session[] sessions = findSessions();
if (sessions != null) {
for (Session session : sessions) {
StandardSession sess = (StandardSession) session;
if (sess.lockBackground()) {
try {
sess.isValid();
} finally {
sess.unlockBackground();
}
}
}
}
Expand Down
Loading

0 comments on commit 7b218e2

Please sign in to comment.