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.

Sign up for GitHub

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

Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT. #596

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Use SELECT...FOR UPDATE to update session data in the database instead of DELETE, INSERT. #596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2c830a6
Use SELECT...FOR UPDATE to update session data in the database instea…
ChristopherSchultz Mar 8, 2023
d5a7ce4
Use ResultSet.insert when inserting. Close ResultSet when done.
ChristopherSchultz Mar 9, 2023
4a12984
Resources in try-with-resources are final.
ChristopherSchultz Mar 9, 2023
bc00845
Allow choice of session-saving strategy via configuration.
ChristopherSchultz Mar 9, 2023
b583071
Only serialize the session once, even if multiple retries are necessary.
ChristopherSchultz Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
220 changes: 208 additions & 12 deletions java/org/apache/catalina/session/DataSourceStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,22 @@
*/
public class DataSourceStore extends StoreBase {

/**
* The DELETE + INSERT saving strategy.
*
* @see #setSaveStrategy
* @see #getSaveStrategy
*/
public static final String SAVE_STRATEGY_DELETE_INSERT = "deleteInsert";

/**
* The SELECT...FOR UPDATE saving strategy.
*
* @see #setSaveStrategy
* @see #getSaveStrategy
*/
public static final String SAVE_STRATEGY_SELECT_FOR_UPDATE = "selectForUpdate";

/**
* Context name associated with this Store
*/
Expand All @@ -75,6 +91,10 @@ public class DataSourceStore extends StoreBase {
*/
protected DataSource dataSource = null;

/**
* Which saving strategy to use: deleteInsert, selectForUpdate, etc.
*/
private String saveStrategy = SAVE_STRATEGY_DELETE_INSERT;

// ------------------------------------------------------------ Table & cols

Expand Down Expand Up @@ -326,6 +346,31 @@ public void setLocalDataSource(boolean localDataSource) {
this.localDataSource = localDataSource;
}

/**
* Sets the session-saving strategy to use.
*
* E.g. {@link #SAVE_STRATEGY_DELETE_INSERT} or {@link #SAVE_STRATEGY_SELECT_FOR_UPDATE}.
*
* The default is {@link #SAVE_STRATEGY_DELETE_INSERT} for compatibility with all RDMBSs.
*
* @param saveStrategy The saving strategy to use.
*/
public void setSaveStrategy(String saveStrategy) {
this.saveStrategy = saveStrategy;
}

/**
* Gets the session-saving strategy to use.
*
* E.g. {@link #SAVE_STRATEGY_DELETE_INSERT} or {@link #SAVE_STRATEGY_SELECT_FOR_UPDATE}.
*
* The default is {@link #SAVE_STRATEGY_DELETE_INSERT} for compatibility with all RDMBSs.
*
* @return The saving strategy to use.
*/
public String getSaveStrategy() {
return saveStrategy;
}

// --------------------------------------------------------- Public Methods

Expand Down Expand Up @@ -596,14 +641,39 @@ public void clear() throws IOException {
*/
@Override
public void save(Session session) throws IOException {
ByteArrayOutputStream bos = null;

byte[] sessionBytes;
try (ByteArrayOutputStream bos = new ByteArrayOutputStream();
ObjectOutputStream oos =
new ObjectOutputStream(new BufferedOutputStream(bos))) {

((StandardSession) session).writeObjectData(oos);

sessionBytes = bos.toByteArray();
}

if(SAVE_STRATEGY_SELECT_FOR_UPDATE.equals(getSaveStrategy())) {
saveSelectForUpdate(session, sessionBytes);
} else {
saveDeleteAndInsert(session, sessionBytes);
}
}

/**
* Saves a session by DELETEing any existing session and INSERTing a new one.
*
* @param session The session to be stored.
* @throws IOException If an input/output error occurs.
*/
private void saveDeleteAndInsert(Session session, byte[] sessionBytes) throws IOException {
String saveSql = "INSERT INTO " + sessionTable + " ("
+ sessionIdCol + ", " + sessionAppCol + ", "
+ sessionDataCol + ", " + sessionValidCol
+ ", " + sessionMaxInactiveCol + ", "
+ sessionLastAccessedCol
+ ") VALUES (?, ?, ?, ?, ?, ?)";

int sessionBytesLength = sessionBytes.length;
synchronized (session) {
int numberOfTries = 2;
while (numberOfTries > 0) {
Expand All @@ -618,19 +688,12 @@ public void save(Session session) throws IOException {
// * Check if ID exists in database and if so use UPDATE.
remove(session.getIdInternal(), _conn);

bos = new ByteArrayOutputStream();
try (ObjectOutputStream oos =
new ObjectOutputStream(new BufferedOutputStream(bos))) {
((StandardSession) session).writeObjectData(oos);
}
byte[] obs = bos.toByteArray();
int size = obs.length;
try (ByteArrayInputStream bis = new ByteArrayInputStream(obs, 0, size);
InputStream in = new BufferedInputStream(bis, size);
PreparedStatement preparedSaveSql = _conn.prepareStatement(saveSql)) {
try (ByteArrayInputStream bis = new ByteArrayInputStream(sessionBytes, 0, sessionBytesLength);
InputStream in = new BufferedInputStream(bis, sessionBytesLength);
PreparedStatement preparedSaveSql = _conn.prepareStatement(saveSql)) {
preparedSaveSql.setString(1, session.getIdInternal());
preparedSaveSql.setString(2, getName());
preparedSaveSql.setBinaryStream(3, in, size);
preparedSaveSql.setBinaryStream(3, in, sessionBytesLength);
preparedSaveSql.setString(4, session.isValid() ? "1" : "0");
preparedSaveSql.setInt(5, session.getMaxInactiveInterval());
preparedSaveSql.setLong(6, session.getLastAccessedTime());
Expand All @@ -655,6 +718,139 @@ public void save(Session session) throws IOException {
}
}

/**
* Saves a session using SELECT ... FOR UPDATE to update any existing session
* record or insert a new one.
*
* This should be more efficient with database resources if it is supported
* by the underlying database.
*
* @param session The session to be stored.
* @throws IOException If an input/output error occurs.
*/
private void saveSelectForUpdate(Session session, byte[] sessionBytes) throws IOException {
String saveSql = "SELECT " + sessionIdCol
+ ", " + sessionAppCol
+ ", " + sessionIdCol
+ ", " + sessionDataCol
+ ", " + sessionValidCol
+ ", " + sessionMaxInactiveCol
+ ", " + sessionLastAccessedCol
+ " FROM " + sessionTable
+ " WHERE " + sessionAppCol + "=?"
+ " AND " + sessionIdCol + "=? FOR UPDATE"
;

int sessionBytesLength = sessionBytes.length;
synchronized (session) {
int numberOfTries = 2;
while (numberOfTries > 0) {
Connection _conn = getConnection();
if (_conn == null) {
return;
}

try {
try (ByteArrayInputStream bis = new ByteArrayInputStream(sessionBytes, 0, sessionBytesLength);
InputStream in = new BufferedInputStream(bis, sessionBytesLength);
PreparedStatement preparedSaveSql = _conn.prepareStatement(saveSql, ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE)) {

// Store auto-commit state
boolean autoCommit = _conn.getAutoCommit();

ResultSet rs = null;
try {
if(autoCommit) {
_conn.setAutoCommit(false); // BEGIN TRANSACTION
}

preparedSaveSql.setString(1, getName());
preparedSaveSql.setString(2, session.getIdInternal());

rs = preparedSaveSql.executeQuery();

if(rs.next()) {
// Session already exists in the db; update the various fields
rs.updateBinaryStream(sessionDataCol, in, sessionBytesLength);
rs.updateString(sessionValidCol, session.isValid() ? "1" : "0");
rs.updateInt(sessionMaxInactiveCol, session.getMaxInactiveInterval());
rs.updateLong(sessionLastAccessedCol, session.getLastAccessedTime());

rs.updateRow();
} else {
// Session does not exist. Insert.
rs.moveToInsertRow();

rs.updateString(sessionAppCol, getName());
rs.updateString(sessionIdCol, session.getIdInternal());
rs.updateBinaryStream(sessionIdCol, in, sessionBytesLength);
rs.updateString(sessionValidCol, session.isValid() ? "1" : "0");
rs.updateInt(sessionMaxInactiveCol, session.getMaxInactiveInterval());
rs.updateLong(sessionLastAccessedCol, session.getLastAccessedTime());

rs.insertRow();
}

_conn.commit();
} catch (SQLException sqle) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to put all the handlers in the same block [1]? e.g.

} catch (SQLException | RuntimeException | Error e) {
    // looks like mostly the same block to me
}

We don't need to support Java 6

[1] https://docs.oracle.com/javase/7/docs/technotes/guides/language/catch-multiple.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be okay merging them together, but we might want to use different error messages. This is one of the things I wanted to get feedback on: how important is it to say "we got an Error" vs "we got an SQLException", etc. when the actual exception and stack trace will likely be logged? I think it's probably okay to merge these exception handlers together but wanted some feedback about the logging.

Copy link
Member

Choose a reason for hiding this comment

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

The Stack Trace would be the same, true, but the exception class and message would still provide the details

manager.getContext().getLogger().error(sm.getString(getStoreName() + ".SQLException", sqle));
try {
_conn.rollback();
} catch (SQLException sqle1) {
manager.getContext().getLogger().error(sm.getString(getStoreName() + ".SQLException", sqle1));
}
} catch (RuntimeException rte) {
manager.getContext().getLogger().error(sm.getString(getStoreName() + ".SQLException", rte));
try {
_conn.rollback();
} catch (SQLException sqle1) {
manager.getContext().getLogger().error(sm.getString(getStoreName() + ".SQLException", sqle1));
}

throw rte;
} catch (Error e) {
manager.getContext().getLogger().error(sm.getString(getStoreName() + ".SQLException", e));
try {
_conn.rollback();
} catch (SQLException sqle1) {
manager.getContext().getLogger().error(sm.getString(getStoreName() + ".SQLException", sqle1));
}

throw e;
} finally {
if(null != rs) {
try {
rs.close();
} catch (SQLException sqle) {
manager.getContext().getLogger().error(sm.getString(getStoreName() + ".SQLException", sqle));
}
}
if(autoCommit) {
// Restore connection auto-commit state
_conn.setAutoCommit(autoCommit);
}
}

// Break out after the finally block
numberOfTries = 0;
}
} catch (SQLException e) {
manager.getContext().getLogger().error(sm.getString(getStoreName() + ".SQLException", e));
} catch (IOException e) {
// Ignore
} finally {
release(_conn);
}
numberOfTries--;
}
}

if (manager.getContext().getLogger().isDebugEnabled()) {
manager.getContext().getLogger().debug(sm.getString(getStoreName() + ".saving",
session.getIdInternal(), sessionTable));
}
}


// --------------------------------------------------------- Protected Methods

Expand Down
7 changes: 7 additions & 0 deletions webapps/docs/config/manager.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,13 @@
<code>false</code>: use a global DataSource.</p>
</attribute>

<attribute name="saveStrategy" required="false">
<p>The session-saving strategy to use, either <code>deleteInsert</code>
or <code>selectForUpdate</code>. The default is <code>deleteInsert</code>
which is compatible with all known RDBMS systems. The <code>selectForUpdate</code>
strategy uses <code>SELECT...FOR UPDATE</code> which may be more efficient
if supported by your RDBMS system.</p>
</attribute>
<attribute name="sessionAppCol" required="false">
<p>Name of the database column, contained in the specified session table,
that contains the Engine, Host, and Web Application Context name in the
Expand Down