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.

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

fix: make Statement.cancel and Statement.setQueryTimeout thread safe #413

Merged
merged 2 commits into from Nov 3, 2015

Conversation

Projects
None yet
3 participants
@vlsi
Member

vlsi commented Oct 31, 2015

Statement.cancel was not thread-safe, thus query timeout from one statement might cancel subsequent statement.

The change introduces state tracking of a statement (ide, in-query, cancelling, cancelled), so out-of-order cancels can be detected and ignored.

There are two basic problems with .cancel handing:

  1. cancels called at random from user code. Even thought Statement#cancel does not require thread-safety, it would be "a bit" unexpected if statementA.cancel would cancel subsequent statementB that is run in the same connection.
    The thing is backend protocol does not allow to cancel a specific execution, thus if user invokes cancel, pgjdbc should ensure that cancel would not terminate another statement by mistake.

The approach is to have statementState and skip cancels that appear when state is "idle".
Additionally, if .execute notices there was a cancel, it waits till cancel completes.

Note: this dance does not guarantee that cancel is indeed dispatched to the backend, however the case when "cancel request" is not dispatched should be very rare. The remaining race is due to OS signals. In other words, current implementation just waits till the "interrupt" signal is sent to the proper backend, however it does not wait till the signal is processed.

  1. setQueryTimeout should not affect another statements or subsequent executions of the same statement.

The approach here is to atomically compareAndSwap cancelTimerTask reference, so the task can detect presence of another task and exit safely.

@vlsi vlsi force-pushed the Gordiychuk:cancel_thread_safety branch from e4a2042 to 034eca6 Oct 31, 2015

@vlsi

This comment has been minimized.

Member

vlsi commented Oct 31, 2015

I think PR is ready for review and merge.

@vlsi vlsi changed the title from Statement.cancel thread safety to fix: make Statement.cancel and Statement.setQueryTimeout thread safe Oct 31, 2015

@davecramer

This comment has been minimized.

Member

davecramer commented Oct 31, 2015

Very elegant solution! Thanks.

Can others please read this over? I'd appreciate more eyes on this.

@@ -768,14 +821,14 @@ public int getQueryTimeout() throws SQLException
* @param seconds - the new query timeout limit in seconds

This comment has been minimized.

@schlosna

schlosna Nov 1, 2015

Contributor

Param is now millis, s/seconds/milliseconds/

This comment has been minimized.

@vlsi

vlsi Nov 1, 2015

Member

good catch

@vlsi vlsi force-pushed the Gordiychuk:cancel_thread_safety branch from 034eca6 to dff861f Nov 1, 2015

@vlsi

This comment has been minimized.

Member

vlsi commented Nov 1, 2015

Question: what is the expected behavior for multiple cancels in a row?
My current change lets only a single cancel per statement execution (subsequent cancels get ignored). Is it an expected behavior? Should multiple cancel requests be allowed?

@davecramer

This comment has been minimized.

Member

davecramer commented Nov 2, 2015

@vlsi I think it's sufficient to just ignore subsequent cancel requests. The use case of cancel is for cancel to be called from a different thread than the one that the statement is currently executing on. I can't see a use case for multiple cancels to be accepted. Furthermore this would add considerable complexity

@vlsi vlsi force-pushed the Gordiychuk:cancel_thread_safety branch from dff861f to 4a8e3b0 Nov 2, 2015

@vlsi

This comment has been minimized.

Member

vlsi commented Nov 2, 2015

Small change: I've changed synchronized(Statement) -> synchronized(Connection) and added synchronized(Connection) for the duration of .cancel.
This makes wait till cancel completes burn less cpu.

@vlsi vlsi force-pushed the Gordiychuk:cancel_thread_safety branch from 4a8e3b0 to bc3d848 Nov 3, 2015

fix: statement.cancel and statement.setQueryTimeout should be thread-…
…safe

Statement.cancel was not thread-safe, thus query timeout from one statement might cancel subsequent statement.
The change introduces state tracking of a statement (idle, in-query, cancelling, cancelled), so out-of-order cancels can be detected and ignored

fixes #412

davecramer added a commit that referenced this pull request Nov 3, 2015

Merge pull request #413 from Gordiychuk/cancel_thread_safety
fix: make Statement.cancel and Statement.setQueryTimeout thread safe

@davecramer davecramer merged commit 000e48e into pgjdbc:master Nov 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vlsi vlsi deleted the Gordiychuk:cancel_thread_safety branch Nov 3, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment