From b18769c1f903d594f3d0097d44eede5b4ef27c52 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Thu, 12 Nov 2015 11:17:20 +1100 Subject: [PATCH] Make setting of last saved time less fragile. --- .../session/AbstractSessionDataStore.java | 12 +- .../session/AbstractSessionIdManager.java | 2 + .../server/session/AbstractSessionStore.java | 25 +-- .../server/session/FileSessionDataStore.java | 2 +- .../server/session/JDBCSessionDataStore.java | 158 +++++++++--------- .../server/session/NullSessionDataStore.java | 2 +- .../session/AbstractForwardedSessionTest.java | 2 + 7 files changed, 99 insertions(+), 104 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionDataStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionDataStore.java index 90702c15931..45b7bc04c62 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionDataStore.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionDataStore.java @@ -44,7 +44,7 @@ public abstract class AbstractSessionDataStore extends AbstractLifeCycle impleme } - public abstract void doStore(SessionKey key, SessionData data) throws Exception; + public abstract void doStore(SessionKey key, SessionData data, boolean isNew) throws Exception; public Context getContext() @@ -65,9 +65,17 @@ public abstract class AbstractSessionDataStore extends AbstractLifeCycle impleme @Override public void store(SessionKey key, SessionData data) throws Exception { + long lastSave = data.getLastSaved(); + + data.setLastSaved(System.currentTimeMillis()); try { - doStore(key, data); + doStore(key, data, (lastSave<=0)); + } + catch (Exception e) + { + //reset last save time + data.setLastSaved(lastSave); } finally { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionIdManager.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionIdManager.java index 09007e80738..6c9a9a71f6b 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionIdManager.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionIdManager.java @@ -373,6 +373,8 @@ public abstract class AbstractSessionIdManager extends AbstractLifeCycle impleme String newClusterId = newSessionId(request.hashCode()); removeId(oldClusterId);//remove the old one from the list (and database) + + useId(newClusterId); //add the new id to list //tell all contexts to update the id Handler[] contexts = _server.getChildHandlersByClass(ContextHandler.class); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionStore.java index f6bd505b58d..017c215e38e 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionStore.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionStore.java @@ -230,29 +230,20 @@ public abstract class AbstractSessionStore extends AbstractLifeCycle implements throw new IllegalArgumentException ("Put key="+key+" session="+(session==null?"null":session.getId())); session.setSessionManager(_manager); + + //if the session data has changed, or the cache is considered stale, write it to any backing store + if ((session.isNew() || session.getSessionData().isDirty() || isStale(session)) && _sessionDataStore != null) + { + session.willPassivate(); + _sessionDataStore.store(key, session.getSessionData()); + session.didActivate(); + } Session existing = doPutIfAbsent(key,session); if (existing == null) { - //session not already in cache write through - if (_sessionDataStore != null) - { - session.willPassivate(); - _sessionDataStore.store(SessionKey.getKey(session.getSessionData()), session.getSessionData()); - session.didActivate(); - } _sessionStats.increment(); } - else - { - //if the session data has changed, or the cache is considered stale, write it to any backing store - if ((session.getSessionData().isDirty() || isStale(session)) && _sessionDataStore != null) - { - session.willPassivate(); - _sessionDataStore.store(key, session.getSessionData()); - session.didActivate(); - } - } } /** diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java index e3918e23921..b189a1e6081 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/FileSessionDataStore.java @@ -228,7 +228,7 @@ public class FileSessionDataStore extends AbstractSessionDataStore * @see org.eclipse.jetty.server.session.AbstractSessionDataStore#doStore(org.eclipse.jetty.server.session.SessionKey, org.eclipse.jetty.server.session.SessionData) */ @Override - public void doStore(SessionKey key, SessionData data) throws Exception + public void doStore(SessionKey key, SessionData data, boolean isNew) throws Exception { File file = null; if (_storeDir != null) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionDataStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionDataStore.java index 857fbbde118..ed9ce14214a 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionDataStore.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/JDBCSessionDataStore.java @@ -308,7 +308,7 @@ public class JDBCSessionDataStore extends AbstractSessionDataStore } } - PreparedStatement statement = connection.prepareStatement("select "+getIdColumn()+", "+", "+getExpiryTimeColumn()+ + PreparedStatement statement = connection.prepareStatement("select "+getIdColumn()+", "+getExpiryTimeColumn()+ " from "+getTableName()+" where "+getContextPathColumn()+" = ? and "+ getVirtualHostColumn()+" = ? and "+ getExpiryTimeColumn()+" >0 and "+getExpiryTimeColumn()+" <= ?"); @@ -636,7 +636,6 @@ public class JDBCSessionDataStore extends AbstractSessionDataStore _sessionTableSchema.setDatabaseAdaptor(_dbAdaptor); _sessionTableSchema.prepareTables(); } - } @@ -741,108 +740,101 @@ public class JDBCSessionDataStore extends AbstractSessionDataStore * @see org.eclipse.jetty.server.session.AbstractSessionDataStore#doStore() */ @Override - public void doStore(SessionKey key, SessionData data) throws Exception + public void doStore(SessionKey key, SessionData data, boolean isNew) throws Exception { if (data==null || key==null) return; - try (Connection connection = _dbAdaptor.getConnection()) - { - connection.setAutoCommit(true); - - //If last saved field not set, then this is a fresh session that has never been persisted - if (data.getLastSaved() <= 0) - { - doInsert(connection, key, data); - } - else - { - doUpdate(connection, key, data); - } - + if (isNew) + { + doInsert(key, data); + } + else + { + doUpdate(key, data); } - } - private void doInsert (Connection connection, SessionKey key, SessionData data) + private void doInsert (SessionKey key, SessionData data) throws Exception { String s = _sessionTableSchema.getInsertSessionStatementAsString(); - try (PreparedStatement statement = connection.prepareStatement(s)) + + try (Connection connection = _dbAdaptor.getConnection()) { + connection.setAutoCommit(true); + try (PreparedStatement statement = connection.prepareStatement(s)) + { + statement.setString(1, key.getId()); //session id + statement.setString(2, key.getCanonicalContextPath()); //context path + statement.setString(3, key.getVhost()); //first vhost + statement.setString(4, data.getLastNode());//my node id + statement.setLong(5, data.getAccessed());//accessTime + statement.setLong(6, data.getLastAccessed()); //lastAccessTime + statement.setLong(7, data.getCreated()); //time created + statement.setLong(8, data.getCookieSet());//time cookie was set + statement.setLong(9, data.getLastSaved()); //last saved time + statement.setLong(10, data.getExpiry()); + statement.setLong(11, data.getMaxInactiveMs()); - long now = System.currentTimeMillis(); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + ObjectOutputStream oos = new ObjectOutputStream(baos); + oos.writeObject(data.getAllAttributes()); + oos.flush(); + byte[] bytes = baos.toByteArray(); - - statement.setString(1, key.getId()); //session id - statement.setString(2, key.getCanonicalContextPath()); //context path - statement.setString(3, key.getVhost()); //first vhost - statement.setString(4, data.getLastNode());//my node id - statement.setLong(5, data.getAccessed());//accessTime - statement.setLong(6, data.getLastAccessed()); //lastAccessTime - statement.setLong(7, data.getCreated()); //time created - statement.setLong(8, data.getCookieSet());//time cookie was set - statement.setLong(9, now); //last saved time - statement.setLong(10, data.getExpiry()); - statement.setLong(11, data.getMaxInactiveMs()); - - ByteArrayOutputStream baos = new ByteArrayOutputStream(); - ObjectOutputStream oos = new ObjectOutputStream(baos); - oos.writeObject(data.getAllAttributes()); - oos.flush(); - byte[] bytes = baos.toByteArray(); - - ByteArrayInputStream bais = new ByteArrayInputStream(bytes); - statement.setBinaryStream(12, bais, bytes.length);//attribute map as blob - statement.executeUpdate(); - data.setLastSaved(now); - if (LOG.isDebugEnabled()) - LOG.debug("Inserted session "+data); + ByteArrayInputStream bais = new ByteArrayInputStream(bytes); + statement.setBinaryStream(12, bais, bytes.length);//attribute map as blob + statement.executeUpdate(); + if (LOG.isDebugEnabled()) + LOG.debug("Inserted session "+data); + } } } - private void doUpdate (Connection connection, SessionKey key, SessionData data) - throws Exception + private void doUpdate (SessionKey key, SessionData data) + throws Exception { - try (PreparedStatement statement = _sessionTableSchema.getUpdateSessionStatement(connection, key.getCanonicalContextPath())) - { - long now = System.currentTimeMillis(); - - statement.setString(1, data.getLastNode());//should be my node id - statement.setLong(2, data.getAccessed());//accessTime - statement.setLong(3, data.getLastAccessed()); //lastAccessTime - statement.setLong(4, now); //last saved time - statement.setLong(5, data.getExpiry()); - statement.setLong(6, data.getMaxInactiveMs()); - - ByteArrayOutputStream baos = new ByteArrayOutputStream(); - ObjectOutputStream oos = new ObjectOutputStream(baos); - oos.writeObject(data.getAllAttributes()); - oos.flush(); - byte[] bytes = baos.toByteArray(); - ByteArrayInputStream bais = new ByteArrayInputStream(bytes); - statement.setBinaryStream(7, bais, bytes.length);//attribute map as blob + try (Connection connection = _dbAdaptor.getConnection()) + { + connection.setAutoCommit(true); + try (PreparedStatement statement = _sessionTableSchema.getUpdateSessionStatement(connection, key.getCanonicalContextPath())) + { + statement.setString(1, data.getLastNode());//should be my node id + statement.setLong(2, data.getAccessed());//accessTime + statement.setLong(3, data.getLastAccessed()); //lastAccessTime + statement.setLong(4, data.getLastSaved()); //last saved time + statement.setLong(5, data.getExpiry()); + statement.setLong(6, data.getMaxInactiveMs()); - if ((key.getCanonicalContextPath() == null || "".equals(key.getCanonicalContextPath())) && _dbAdaptor.isEmptyStringNull()) - { - statement.setString(8, key.getId()); - statement.setString(9, key.getVhost()); - } - else - { - statement.setString(8, key.getId()); - statement.setString(9, key.getCanonicalContextPath()); - statement.setString(10, key.getVhost()); - } - - statement.executeUpdate(); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + ObjectOutputStream oos = new ObjectOutputStream(baos); + oos.writeObject(data.getAllAttributes()); + oos.flush(); + byte[] bytes = baos.toByteArray(); + ByteArrayInputStream bais = new ByteArrayInputStream(bytes); + statement.setBinaryStream(7, bais, bytes.length);//attribute map as blob - data.setLastSaved(now); - if (LOG.isDebugEnabled()) - LOG.debug("Updated session "+data); - } + if ((key.getCanonicalContextPath() == null || "".equals(key.getCanonicalContextPath())) && _dbAdaptor.isEmptyStringNull()) + { + statement.setString(8, key.getId()); + statement.setString(9, key.getVhost()); + } + else + { + statement.setString(8, key.getId()); + statement.setString(9, key.getCanonicalContextPath()); + statement.setString(10, key.getVhost()); + } + + statement.executeUpdate(); + + if (LOG.isDebugEnabled()) + LOG.debug("Updated session "+data); + } + } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/NullSessionDataStore.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/NullSessionDataStore.java index 32b7f078465..67bc4c1a18a 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/NullSessionDataStore.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/NullSessionDataStore.java @@ -61,7 +61,7 @@ public class NullSessionDataStore extends AbstractSessionDataStore * @see org.eclipse.jetty.server.session.AbstractSessionDataStore#doStore() */ @Override - public void doStore(SessionKey key, SessionData data) throws Exception + public void doStore(SessionKey key, SessionData data, boolean isNew) throws Exception { //noop } diff --git a/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractForwardedSessionTest.java b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractForwardedSessionTest.java index e5e6c6ba7a2..743ca494fa6 100644 --- a/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractForwardedSessionTest.java +++ b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractForwardedSessionTest.java @@ -126,6 +126,7 @@ public abstract class AbstractForwardedSessionTest HttpSession sess = request.getSession(false); assertNotNull(sess); + assertNotNull(sess.getAttribute("servlet3")); sess.setAttribute("servlet1", "servlet1"); } } @@ -144,6 +145,7 @@ public abstract class AbstractForwardedSessionTest //the session should exist after the forward HttpSession sess = request.getSession(false); assertNotNull(sess); + assertNotNull(sess.getAttribute("servlet3")); } }