Skip to content

Commit 42eb56c

Browse files
magnuspmagnusp
authored
fix(jdbc): serialize statement close and execute (#89)
Closes #49. Part of #54. ## What changed - `LibsqlStatement` state transitions (`execute`, `executeQuery`, `close`, result-set installation and removal, generated keys, `getResultSet`, `isClosed`, `closeOnCompletion`) now share one private lock. - `execute`/`executeQuery` perform the network call outside the lock, re-check `closed` under the lock, and only attach the result set if close has not won. If close won, the produced result set is closed and not attached. - `close()` closes the current result set and generated keys while holding the lock, then marks the statement closed. - `LibsqlPreparedStatement` reuses the same `assignResultSet`/`executeResult` paths, so prepared statements get the same guarantee. - Added `LibsqlStatementConcurrencyTest` with a deterministic latch seam (`beforeResultSetAssignment`) between the closed check and result-set installation. The test starts close while execute is paused in the seam, so the race does not depend on timing luck. ## Contract decision This is a correctness fix, not an xerial parity change. #47 deliberately keeps execute-after-close working; that remains true: the call still executes and does not throw. The one observable tightening is that a result set produced after the statement is already closed is returned closed instead of open. This is required by #49's acceptance criterion that a closed statement never holds an open result set after execute returns, and it also covers the reproduction where close completes before the next execute starts. ## Evidence Failing before the fix (seam and tests present, locking absent): ``` ./mvnw -pl libsql-jdbc -am -Dtest=LibsqlStatementConcurrencyTest -Dsurefire.failIfNoSpecifiedTests=false test Tests run: 3, Failures: 3, Errors: 0, Skipped: 0 plainExecuteQueryDoesNotAttachAnOpenResultSetWhenCloseWinsTheRace preparedExecuteQueryDoesNotAttachAnOpenResultSetWhenCloseWinsTheRace executeQueryOnAClosedStatementReturnsAClosedResultSet ``` Passing after the fix: ``` ./mvnw -pl libsql-jdbc -am test Tests run: 165, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` The concurrency test was also run five times in a row, all green. `./mvnw spotless:apply` and `./mvnw spotless:check` pass. ## Deviations None for the Hrana transport or sqld. The only intentional behavior change beyond the race is closing a result set produced by an execute on an already-closed statement, described under Contract decision.
1 parent 97dd749 commit 42eb56c

3 files changed

Lines changed: 341 additions & 98 deletions

File tree

‎libsql-jdbc/src/main/java/com/github/magnusp/libsql/jdbc/LibsqlPreparedStatement.java‎

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,9 @@ public ResultSet executeQuery() throws SQLException {
6464
if (result.cols() == null || result.cols().isEmpty()) {
6565
throw new SQLException("Query does not return results");
6666
}
67-
this.currentResultSet = new LibsqlResultSet(this, result, sql, false);
68-
this.updateCount = -1;
69-
return currentResultSet;
67+
LibsqlResultSet resultSet = new LibsqlResultSet(this, result, sql, false);
68+
assignResultSet(resultSet, false);
69+
return resultSet;
7070
}
7171

7272
@Override
@@ -217,17 +217,7 @@ private record Token(String word, int depth) {}
217217

218218
@Override
219219
public boolean execute() throws SQLException {
220-
StmtResult result = executeCurrent();
221-
updateGeneratedKeys(sql, result);
222-
if (result.cols() != null && !result.cols().isEmpty()) {
223-
this.currentResultSet = new LibsqlResultSet(this, result, sql, false);
224-
this.updateCount = -1;
225-
return true;
226-
} else {
227-
this.currentResultSet = null;
228-
this.updateCount = result.affectedRowCount();
229-
return false;
230-
}
220+
return executeResult(sql, executeCurrent());
231221
}
232222

233223
private StmtResult executeCurrent() throws SQLException {

‎libsql-jdbc/src/main/java/com/github/magnusp/libsql/jdbc/LibsqlStatement.java‎

Lines changed: 162 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,17 @@
3737
* <li>Matching xerial's execute-after-close behavior, {@link #execute(String)}, {@link
3838
* #executeQuery(String)}, {@link #executeUpdate(String)}, and {@link #getConnection()} keep
3939
* working after {@link #close()}. This deliberately copies a likely xerial bug for drop-in
40-
* compatibility; see the issue #47 contract decision.
40+
* compatibility; see the issue #47 contract decision. A result set produced after close is
41+
* returned closed, so a closed statement never holds an open result set.
42+
* <li>{@link #close()} and the execute methods are safe when called concurrently. State
43+
* transitions share one lock; when close wins a race, the execute closes and discards its
44+
* result set instead of attaching it to the closed statement.
4145
* </ul>
4246
*/
4347
public class LibsqlStatement implements Statement {
4448

4549
protected final LibsqlConnection connection;
50+
private final Object stateLock = new Object();
4651
protected LibsqlResultSet currentResultSet = null;
4752
protected long updateCount = -1;
4853
protected boolean closed = false;
@@ -56,9 +61,17 @@ public LibsqlStatement(LibsqlConnection connection) {
5661
this.connection = connection;
5762
}
5863

64+
/**
65+
* Test seam. Called after the closed check and before a result set is attached to the
66+
* statement, so a test can force a close to interleave with execute.
67+
*/
68+
protected void beforeResultSetAssignment() throws SQLException {}
69+
5970
protected void checkClosed() throws SQLException {
60-
if (closed) {
61-
throw new SQLException("Statement is closed");
71+
synchronized (stateLock) {
72+
if (closed) {
73+
throw new SQLException("Statement is closed");
74+
}
6275
}
6376
if (connection != null) {
6477
connection.checkClosed();
@@ -71,10 +84,28 @@ public ResultSet executeQuery(String sql) throws SQLException {
7184
if (result.cols() == null || result.cols().isEmpty()) {
7285
throw new SQLException("Query did not produce a ResultSet: " + sql);
7386
}
74-
this.currentResultSet = new LibsqlResultSet(this, result, sql, false);
75-
this.updateCount = -1;
76-
this.resultSetRequested = true;
77-
return currentResultSet;
87+
LibsqlResultSet resultSet = new LibsqlResultSet(this, result, sql, false);
88+
assignResultSet(resultSet, true);
89+
return resultSet;
90+
}
91+
92+
protected void assignResultSet(LibsqlResultSet resultSet, boolean requested)
93+
throws SQLException {
94+
if (!isClosed()) {
95+
beforeResultSetAssignment();
96+
}
97+
synchronized (stateLock) {
98+
if (closed) {
99+
closeResultSet(resultSet);
100+
currentResultSet = null;
101+
resultSetRequested = false;
102+
updateCount = -1;
103+
return;
104+
}
105+
currentResultSet = resultSet;
106+
updateCount = -1;
107+
resultSetRequested = requested;
108+
}
78109
}
79110

80111
@Override
@@ -85,18 +116,34 @@ public int executeUpdate(String sql) throws SQLException {
85116

86117
@Override
87118
public boolean execute(String sql) throws SQLException {
88-
StmtResult result = executeSql(sql);
89-
updateGeneratedKeys(sql, result);
90-
if (result.cols() != null && !result.cols().isEmpty()) {
91-
this.currentResultSet = new LibsqlResultSet(this, result, sql, false);
92-
this.updateCount = -1;
93-
this.resultSetRequested = false;
94-
return true;
95-
} else {
96-
this.currentResultSet = null;
97-
this.updateCount = result.affectedRowCount();
98-
this.resultSetRequested = false;
99-
return false;
119+
return executeResult(sql, executeSql(sql));
120+
}
121+
122+
protected boolean executeResult(String sql, StmtResult result) throws SQLException {
123+
boolean hasResultSet = result.cols() != null && !result.cols().isEmpty();
124+
LibsqlResultSet resultSet =
125+
hasResultSet ? new LibsqlResultSet(this, result, sql, false) : null;
126+
127+
if (hasResultSet && !isClosed()) {
128+
beforeResultSetAssignment();
129+
}
130+
131+
synchronized (stateLock) {
132+
if (closed) {
133+
clearGeneratedKeys();
134+
if (resultSet != null) {
135+
closeResultSet(resultSet);
136+
}
137+
currentResultSet = null;
138+
resultSetRequested = false;
139+
updateCount = hasResultSet ? -1 : result.affectedRowCount();
140+
return hasResultSet;
141+
}
142+
updateGeneratedKeys(sql, result);
143+
currentResultSet = resultSet;
144+
updateCount = hasResultSet ? -1 : result.affectedRowCount();
145+
resultSetRequested = false;
146+
return hasResultSet;
100147
}
101148
}
102149

@@ -106,44 +153,58 @@ private StmtResult executeSql(String sql) throws SQLException {
106153
}
107154

108155
protected void closeCurrentResultSet() throws SQLException {
109-
LibsqlResultSet resultSet = currentResultSet;
110-
currentResultSet = null;
111-
resultSetRequested = false;
112-
if (resultSet != null) {
113-
closeResultSet(resultSet);
156+
synchronized (stateLock) {
157+
LibsqlResultSet resultSet = currentResultSet;
158+
currentResultSet = null;
159+
resultSetRequested = false;
160+
if (resultSet != null) {
161+
closeResultSet(resultSet);
162+
}
114163
}
115164
}
116165

117166
private void closeResultSet(LibsqlResultSet resultSet) throws SQLException {
118-
boolean wasClosed = closed;
119-
boolean wasClosing = closing;
120-
closed = false;
121-
closing = true;
122-
try {
123-
resultSet.close();
124-
} finally {
125-
closed = wasClosed;
126-
closing = wasClosing;
167+
synchronized (stateLock) {
168+
boolean wasClosed = closed;
169+
boolean wasClosing = closing;
170+
closed = false;
171+
closing = true;
172+
try {
173+
resultSet.close();
174+
} finally {
175+
closed = wasClosed;
176+
closing = wasClosing;
177+
}
127178
}
128179
}
129180

130181
protected void updateGeneratedKeys(String sql, StmtResult result) throws SQLException {
131-
clearGeneratedKeys();
132-
if (sql != null && result != null && result.lastInsertRowid() != null && isInsertSql(sql)) {
133-
long rowid = result.lastInsertRowid();
134-
StmtResult keys = new StmtResult(
135-
List.of(new Col("last_insert_rowid()", null)),
136-
List.of(new Row(List.of(Value.of(rowid)))),
137-
0,
138-
rowid);
139-
generatedKeys = new LibsqlResultSet(this, keys);
182+
synchronized (stateLock) {
183+
clearGeneratedKeys();
184+
if (sql != null
185+
&& result != null
186+
&& result.lastInsertRowid() != null
187+
&& isInsertSql(sql)) {
188+
long rowid = result.lastInsertRowid();
189+
StmtResult keys = new StmtResult(
190+
List.of(new Col("last_insert_rowid()", null)),
191+
List.of(new Row(List.of(Value.of(rowid)))),
192+
0,
193+
rowid);
194+
generatedKeys = new LibsqlResultSet(this, keys);
195+
}
140196
}
141197
}
142198

143199
protected void clearGeneratedKeys() throws SQLException {
144-
if (generatedKeys != null) {
145-
generatedKeys.close();
200+
synchronized (stateLock) {
201+
ResultSet keys = generatedKeys;
146202
generatedKeys = null;
203+
if (keys instanceof LibsqlResultSet libsqlKeys) {
204+
closeResultSet(libsqlKeys);
205+
} else if (keys != null) {
206+
keys.close();
207+
}
147208
}
148209
}
149210

@@ -244,35 +305,43 @@ private ResultSet emptyGeneratedKeys() {
244305

245306
@Override
246307
public ResultSet getResultSet() throws SQLException {
247-
checkClosed();
248-
if (currentResultSet == null) {
249-
return null;
250-
}
251-
if (resultSetRequested) {
252-
throw new SQLException("ResultSet already requested");
308+
synchronized (stateLock) {
309+
checkClosed();
310+
if (currentResultSet == null) {
311+
return null;
312+
}
313+
if (resultSetRequested) {
314+
throw new SQLException("ResultSet already requested");
315+
}
316+
resultSetRequested = true;
317+
return currentResultSet;
253318
}
254-
resultSetRequested = true;
255-
return currentResultSet;
256319
}
257320

258321
@Override
259322
public int getUpdateCount() throws SQLException {
260-
checkClosed();
261-
return (int) updateCount;
323+
synchronized (stateLock) {
324+
checkClosed();
325+
return (int) updateCount;
326+
}
262327
}
263328

264329
@Override
265330
public long getLargeUpdateCount() throws SQLException {
266-
checkClosed();
267-
return updateCount;
331+
synchronized (stateLock) {
332+
checkClosed();
333+
return updateCount;
334+
}
268335
}
269336

270337
@Override
271338
public boolean getMoreResults() throws SQLException {
272-
checkClosed();
273-
closeCurrentResultSet();
274-
updateCount = -1;
275-
return false;
339+
synchronized (stateLock) {
340+
checkClosed();
341+
closeCurrentResultSet();
342+
updateCount = -1;
343+
return false;
344+
}
276345
}
277346

278347
@Override
@@ -304,28 +373,31 @@ public Connection getConnection() throws SQLException {
304373

305374
@Override
306375
public void close() throws SQLException {
307-
if (closing) {
308-
return;
309-
}
310-
closing = true;
311-
try {
312-
closed = false;
313-
clearGeneratedKeys();
314-
LibsqlResultSet resultSet = currentResultSet;
315-
currentResultSet = null;
316-
resultSetRequested = false;
317-
if (resultSet != null) {
318-
resultSet.close();
376+
synchronized (stateLock) {
377+
if (closing) {
378+
return;
379+
}
380+
closing = true;
381+
try {
382+
LibsqlResultSet resultSet = currentResultSet;
383+
currentResultSet = null;
384+
resultSetRequested = false;
385+
clearGeneratedKeys();
386+
if (resultSet != null) {
387+
closeResultSet(resultSet);
388+
}
389+
} finally {
390+
closed = true;
391+
closing = false;
319392
}
320-
} finally {
321-
closed = true;
322-
closing = false;
323393
}
324394
}
325395

326396
@Override
327397
public boolean isClosed() throws SQLException {
328-
return closed;
398+
synchronized (stateLock) {
399+
return closed;
400+
}
329401
}
330402

331403
@Override public int getMaxFieldSize() throws SQLException { return 0; }
@@ -359,11 +431,13 @@ public boolean getMoreResults(int current) throws SQLException {
359431
}
360432
@Override
361433
public ResultSet getGeneratedKeys() throws SQLException {
362-
checkClosed();
363-
if (generatedKeys == null) {
364-
generatedKeys = emptyGeneratedKeys();
434+
synchronized (stateLock) {
435+
checkClosed();
436+
if (generatedKeys == null) {
437+
generatedKeys = emptyGeneratedKeys();
438+
}
439+
return generatedKeys;
365440
}
366-
return generatedKeys;
367441
}
368442
@Override public int executeUpdate(String sql, int autoGeneratedKeys) throws SQLException { return executeUpdate(sql); }
369443
@Override public int executeUpdate(String sql, int[] columnIndexes) throws SQLException { return executeUpdate(sql); }
@@ -376,14 +450,18 @@ public ResultSet getGeneratedKeys() throws SQLException {
376450
@Override public boolean isPoolable() throws SQLException { return false; }
377451
@Override
378452
public void closeOnCompletion() throws SQLException {
379-
checkClosedForCompletion();
380-
closeOnCompletion = true;
453+
synchronized (stateLock) {
454+
checkClosedForCompletion();
455+
closeOnCompletion = true;
456+
}
381457
}
382458

383459
@Override
384460
public boolean isCloseOnCompletion() throws SQLException {
385-
checkClosedForCompletion();
386-
return closeOnCompletion;
461+
synchronized (stateLock) {
462+
checkClosedForCompletion();
463+
return closeOnCompletion;
464+
}
387465
}
388466

389467
private void checkClosedForCompletion() throws SQLException {

0 commit comments

Comments
 (0)