From d3379029bbfcbccb21bc470c3598cdadcb9b3919 Mon Sep 17 00:00:00 2001 From: Vedran Pavic Date: Thu, 8 Sep 2016 00:04:36 +0200 Subject: [PATCH] Fix custom expire for JdbcOperationsSessionRepository.cleanUpExpiredSessions Currently, JdbcOperationsSessionRepository#cleanUpExpiredSessions only considers the repository defined max inactive interval which causes incorrect cleanup of sessions that define custom inactive interval. This commit fixes the problem by delegating calculation of deletion interval to the underlying SQL DELETE statement. Fixes gh-580 --- ...JdbcOperationsSessionRepositoryITests.java | 51 ++++++++++++------- .../jdbc/JdbcOperationsSessionRepository.java | 17 +------ .../http/JdbcHttpSessionConfiguration.java | 23 ++++++++- .../JdbcOperationsSessionRepositoryTests.java | 17 ------- 4 files changed, 57 insertions(+), 51 deletions(-) diff --git a/spring-session/src/integration-test/java/org/springframework/session/jdbc/AbstractJdbcOperationsSessionRepositoryITests.java b/spring-session/src/integration-test/java/org/springframework/session/jdbc/AbstractJdbcOperationsSessionRepositoryITests.java index 4b9757bc..851e3297 100644 --- a/spring-session/src/integration-test/java/org/springframework/session/jdbc/AbstractJdbcOperationsSessionRepositoryITests.java +++ b/spring-session/src/integration-test/java/org/springframework/session/jdbc/AbstractJdbcOperationsSessionRepositoryITests.java @@ -18,6 +18,7 @@ package org.springframework.session.jdbc; import java.util.Map; import java.util.UUID; +import java.util.concurrent.TimeUnit; import javax.sql.DataSource; @@ -501,9 +502,7 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { } @Test - public void cleanupCleansUpInactiveSessions() { - this.repository.setDefaultMaxInactiveInterval(1800); // 30 minutes - + public void cleanupInactiveSessionsUsingRepositoryDefinedInterval() { JdbcOperationsSessionRepository.JdbcSession session = this.repository .createSession(); @@ -513,32 +512,50 @@ public abstract class AbstractJdbcOperationsSessionRepositoryITests { this.repository.cleanUpExpiredSessions(); - // verify its not cleaned up immediately after creation assertThat(this.repository.getSession(session.getId())).isNotNull(); long now = System.currentTimeMillis(); - long tenMinutesInMilliseconds = 1000 * 60 * 10; - long thirtyMinutesInMilliseconds = 1000 * 60 * 30; - // set the last accessed time to 10 minutes in the past, session should not get - // cleaned up because it has not been inactive for 30 minutes yet - long tenMinutesInThePast = now - tenMinutesInMilliseconds; - session.setLastAccessedTime(tenMinutesInThePast); + session.setLastAccessedTime(now - TimeUnit.MINUTES.toMillis(10)); this.repository.save(session); - this.repository.cleanUpExpiredSessions(); - // session should still exist assertThat(this.repository.getSession(session.getId())).isNotNull(); - // set the last accessed time to 30 minutes in the past, session should get - // cleaned up as it has been inactive for 30 minutes - long thirtyMinutesInThePast = now - thirtyMinutesInMilliseconds; - session.setLastAccessedTime(thirtyMinutesInThePast); + session.setLastAccessedTime(now - TimeUnit.MINUTES.toMillis(30)); + this.repository.save(session); + this.repository.cleanUpExpiredSessions(); + + assertThat(this.repository.getSession(session.getId())).isNull(); + } + + // gh-580 + @Test + public void cleanupInactiveSessionsUsingSessionDefinedInterval() { + JdbcOperationsSessionRepository.JdbcSession session = this.repository + .createSession(); + session.setMaxInactiveIntervalInSeconds((int) TimeUnit.MINUTES.toSeconds(45)); + this.repository.save(session); + assertThat(this.repository.getSession(session.getId())).isNotNull(); + this.repository.cleanUpExpiredSessions(); - // session should have been cleaned up + + assertThat(this.repository.getSession(session.getId())).isNotNull(); + + long now = System.currentTimeMillis(); + + session.setLastAccessedTime(now - TimeUnit.MINUTES.toMillis(40)); + this.repository.save(session); + this.repository.cleanUpExpiredSessions(); + + assertThat(this.repository.getSession(session.getId())).isNotNull(); + + session.setLastAccessedTime(now - TimeUnit.MINUTES.toMillis(50)); + this.repository.save(session); + this.repository.cleanUpExpiredSessions(); + assertThat(this.repository.getSession(session.getId())).isNull(); } diff --git a/spring-session/src/main/java/org/springframework/session/jdbc/JdbcOperationsSessionRepository.java b/spring-session/src/main/java/org/springframework/session/jdbc/JdbcOperationsSessionRepository.java index 129fc05a..a1d38dc4 100644 --- a/spring-session/src/main/java/org/springframework/session/jdbc/JdbcOperationsSessionRepository.java +++ b/spring-session/src/main/java/org/springframework/session/jdbc/JdbcOperationsSessionRepository.java @@ -21,7 +21,6 @@ import java.sql.ResultSet; import java.sql.SQLException; import java.util.ArrayList; import java.util.Collections; -import java.util.Date; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -177,7 +176,7 @@ public class JdbcOperationsSessionRepository implements private static final String DELETE_SESSIONS_BY_LAST_ACCESS_TIME_QUERY = "DELETE FROM %TABLE_NAME% " + - "WHERE LAST_ACCESS_TIME < ?"; + "WHERE LAST_ACCESS_TIME < ? - MAX_INACTIVE_INTERVAL * 1000"; private static final Log logger = LogFactory .getLog(JdbcOperationsSessionRepository.class); @@ -564,24 +563,12 @@ public class JdbcOperationsSessionRepository implements @Scheduled(cron = "0 * * * * *") public void cleanUpExpiredSessions() { - long now = System.currentTimeMillis(); - long maxInactiveIntervalSeconds = (this.defaultMaxInactiveInterval != null) - ? this.defaultMaxInactiveInterval - : MapSession.DEFAULT_MAX_INACTIVE_INTERVAL_SECONDS; - - final long sessionsValidFromTime = now - (maxInactiveIntervalSeconds * 1000); - - if (logger.isDebugEnabled()) { - logger.debug( - "Cleaning up sessions older than " + new Date(sessionsValidFromTime)); - } - int deletedCount = this.transactionOperations.execute(new TransactionCallback() { public Integer doInTransaction(TransactionStatus transactionStatus) { return JdbcOperationsSessionRepository.this.jdbcOperations.update( JdbcOperationsSessionRepository.this.deleteSessionsByLastAccessTimeQuery, - sessionsValidFromTime); + System.currentTimeMillis()); } }); diff --git a/spring-session/src/main/java/org/springframework/session/jdbc/config/annotation/web/http/JdbcHttpSessionConfiguration.java b/spring-session/src/main/java/org/springframework/session/jdbc/config/annotation/web/http/JdbcHttpSessionConfiguration.java index 2eda6dbe..ccda2442 100644 --- a/spring-session/src/main/java/org/springframework/session/jdbc/config/annotation/web/http/JdbcHttpSessionConfiguration.java +++ b/spring-session/src/main/java/org/springframework/session/jdbc/config/annotation/web/http/JdbcHttpSessionConfiguration.java @@ -15,6 +15,7 @@ */ package org.springframework.session.jdbc.config.annotation.web.http; +import java.util.Arrays; import java.util.Map; import javax.sql.DataSource; @@ -33,6 +34,8 @@ import org.springframework.core.serializer.support.SerializingConverter; import org.springframework.core.type.AnnotationMetadata; import org.springframework.jdbc.core.JdbcOperations; import org.springframework.jdbc.core.JdbcTemplate; +import org.springframework.jdbc.support.JdbcUtils; +import org.springframework.jdbc.support.MetaDataAccessException; import org.springframework.jdbc.support.lob.LobHandler; import org.springframework.scheduling.annotation.EnableScheduling; import org.springframework.session.config.annotation.web.http.SpringHttpSessionConfiguration; @@ -80,14 +83,19 @@ public class JdbcHttpSessionConfiguration extends SpringHttpSessionConfiguration @Bean public JdbcOperationsSessionRepository sessionRepository( - @Qualifier("springSessionJdbcOperations") JdbcOperations jdbcOperations, + @Qualifier("springSessionJdbcOperations") JdbcTemplate jdbcTemplate, PlatformTransactionManager transactionManager) { JdbcOperationsSessionRepository sessionRepository = - new JdbcOperationsSessionRepository(jdbcOperations, transactionManager); + new JdbcOperationsSessionRepository(jdbcTemplate, transactionManager); String tableName = getTableName(); if (StringUtils.hasText(tableName)) { sessionRepository.setTableName(tableName); } + String databaseName = getDatabaseName(jdbcTemplate.getDataSource()); + if (Arrays.asList("Apache Derby", "H2").contains(databaseName)) { + sessionRepository.setDeleteSessionsByLastAccessTimeQuery("DELETE FROM " + tableName + + " WHERE LAST_ACCESS_TIME < ? - CAST(MAX_INACTIVE_INTERVAL AS BIGINT) * 1000"); + } sessionRepository .setDefaultMaxInactiveInterval(this.maxInactiveIntervalInSeconds); if (this.lobHandler != null) { @@ -150,6 +158,17 @@ public class JdbcHttpSessionConfiguration extends SpringHttpSessionConfiguration this.maxInactiveIntervalInSeconds = maxInactiveIntervalInSeconds; } + private String getDatabaseName(DataSource dataSource) { + try { + String databaseProductName = JdbcUtils.extractDatabaseMetaData(dataSource, + "getDatabaseProductName").toString(); + return JdbcUtils.commonDatabaseName(databaseProductName); + } + catch (MetaDataAccessException e) { + return null; + } + } + private String getTableName() { String systemProperty = System.getProperty("spring.session.jdbc.tableName", ""); if (StringUtils.hasText(systemProperty)) { diff --git a/spring-session/src/test/java/org/springframework/session/jdbc/JdbcOperationsSessionRepositoryTests.java b/spring-session/src/test/java/org/springframework/session/jdbc/JdbcOperationsSessionRepositoryTests.java index 4832fe9e..bfc8ff1d 100644 --- a/spring-session/src/test/java/org/springframework/session/jdbc/JdbcOperationsSessionRepositoryTests.java +++ b/spring-session/src/test/java/org/springframework/session/jdbc/JdbcOperationsSessionRepositoryTests.java @@ -22,7 +22,6 @@ import java.util.Map; import javax.sql.DataSource; -import org.assertj.core.data.Offset; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -524,22 +523,6 @@ public class JdbcOperationsSessionRepositoryTests { verify(this.jdbcOperations, times(1)).update(startsWith("DELETE"), anyLong()); } - // gh-564 - @Test - public void cleanupExpiredSessionsNoOverflow() { - long now = System.currentTimeMillis(); - long toDelete = now - (new Long(Integer.MAX_VALUE) * 1000L); - this.repository.setDefaultMaxInactiveInterval(Integer.MAX_VALUE); - - this.repository.cleanUpExpiredSessions(); - - ArgumentCaptor time = ArgumentCaptor.forClass(Long.class); - assertPropagationRequiresNew(); - verify(this.jdbcOperations, times(1)).update(startsWith("DELETE"), - time.capture()); - assertThat(time.getValue()).isCloseTo(toDelete, Offset.offset(5L)); - } - private void assertPropagationRequiresNew() { ArgumentCaptor argument = ArgumentCaptor.forClass(TransactionDefinition.class);