Skip to content

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

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

Open
ChristopherSchultz 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

Changes from 1 commit
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
102 changes: 82 additions & 20 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 @@ -597,12 +597,17 @@ public void clear() throws IOException {
@Override
public void save(Session session) throws IOException {
ByteArrayOutputStream bos = null;
String saveSql = "INSERT INTO " + sessionTable + " ("
+ sessionIdCol + ", " + sessionAppCol + ", "
+ sessionDataCol + ", " + sessionValidCol
+ ", " + sessionMaxInactiveCol + ", "
+ sessionLastAccessedCol
+ ") VALUES (?, ?, ?, ?, ?, ?)";
String saveSql = "SELECT " + sessionIdCol
+ ", " + sessionAppCol
+ ", " + sessionIdCol
+ ", " + sessionDataCol
+ ", " + sessionValidCol
+ ", " + sessionMaxInactiveCol
+ ", " + sessionLastAccessedCol
+ " FROM " + sessionTable
+ " WHERE " + sessionAppCol + "=?"
+ " AND " + sessionIdCol + "=? FOR UPDATE"
;

synchronized (session) {
int numberOfTries = 2;
Expand All @@ -613,11 +618,6 @@ public void save(Session session) throws IOException {
}

try {
// If sessions already exist in DB, remove and insert again.
// TODO:
// * 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))) {
Expand All @@ -626,15 +626,77 @@ public void save(Session session) throws IOException {
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)) {
preparedSaveSql.setString(1, session.getIdInternal());
preparedSaveSql.setString(2, getName());
preparedSaveSql.setBinaryStream(3, in, size);
preparedSaveSql.setString(4, session.isValid() ? "1" : "0");
preparedSaveSql.setInt(5, session.getMaxInactiveInterval());
preparedSaveSql.setLong(6, session.getLastAccessedTime());
preparedSaveSql.execute();
InputStream in = new BufferedInputStream(bis, size);
PreparedStatement preparedSaveSql = _conn.prepareStatement(saveSql, ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE)) {

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

try {
if(autoCommit) {
_conn.setAutoCommit(false); // BEGIN TRANSACTION
}

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

ResultSet rs = preparedSaveSql.executeQuery();

if(rs.next()) {
// Session already exists in the db; update the various fields
rs.updateBinaryStream(sessionDataCol, in, size);
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, size);
rs.updateString(sessionValidCol, session.isValid() ? "1" : "0");
rs.updateInt(sessionMaxInactiveCol, session.getMaxInactiveInterval());
rs.updateLong(sessionLastAccessedCol, session.getLastAccessedTime());

rs.updateRow();
Comment thread
ChristopherSchultz marked this conversation as resolved.
Outdated
}

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

@isapir isapir Mar 8, 2023

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
Copy Markdown
Contributor Author

@ChristopherSchultz ChristopherSchultz Mar 9, 2023

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
Copy Markdown
Member

@isapir isapir Mar 9, 2023

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(autoCommit) {
// Restore connection auto-commit state
_conn.setAutoCommit(autoCommit);
}
}

// Break out after the finally block
numberOfTries = 0;
}
Expand Down