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.

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

remove receiving EOF from backend after cancel … #1641

Merged
merged 1 commit into from Dec 16, 2019

Conversation

mahmoudbahaa
Copy link
Contributor

@mahmoudbahaa mahmoudbahaa commented Dec 6, 2019

since according to protocol the server closes the connection immediately once cancel is sent (connection reset exception is always thrown) and that was slowing the close considerably (the 1000 close used to take 22 seconds because of this now it takes 1-1.5 seconds on my machine. I also changed the timeout of the test from 20 seconds to 2 seconds.

quote:

The server will process this [cancel] request and then close the connection. For security reasons, no direct reply is made to the cancel request message

." https://www.postgresql.org/docs/current/protocol-flow.html

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Does mvn checkstyle:check pass ?
  3. Have you added your new test classes to an existing test suite in alphabetical order?

Changes to Existing Features:

  • Does this break existing behaviour? If so please explain.
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

…tocol the server closes the connection once cancel is sent (connection reset exception is always thrown)

quote "The server will process this [cancel] request and then close the connection. For security reasons, no direct reply is made to the cancel request message." https://www.postgresql.org/docs/current/protocol-flow.html
@davecramer davecramer merged commit 23cce8a into pgjdbc:master Dec 16, 2019
0 of 2 checks passed
@vlsi
Copy link
Member

@vlsi vlsi commented Mar 4, 2020

@mahmoudbahaa , this change breaks org.postgresql.test.jdbc2.StatementTest#testShortQueryTimeout

Can you please elaborate on why do you need the change?

TL;DR: I'm reverting this change (and aaccf43) in 48 hours unless there's a clarification.


Just in case: testShortQueryTimeout verifies the following case:

statementWithTimeout.setQueryTimeoutMs(1);

try { statementWithTimeout.executeQuery("select 1"); } catch (Throwable ignored) { }
statement.executeQuery("select 1");

Of course, statementWithTimeout might fail, however, statement must not fail with `timeout exception because the user has never set a timeout for it.

Unfortunately, we don't have query identifier in PostgreSQL, so the only thing we can do is to set cancel request to the backend, and wait for it to process.
Unfortunately^2 cancels in the backend are implemented via POSIX signals, so they are asynchronous, so noticeable time might pass until the backend receives the cancel.

cancelStream.receiveEOF(); was there to ensure backend does receive the cancel request. Of course, due to security reasons we don't know if something was canceled or not, but we do know we used the right session identifier for the cancel ticket, so we know backend tries to cancel the query.

We don't really want the following:

// start executing the query 
try { statementWithTimeout.executeQuery("select 1"); } catch (Throwable ignored) { }
// a background thread creates a timer to send `cancel` after `1ms`
// 1ms passes. The query is already completed, but the response is buffered somewhere
// background thread identifies that the query is still in progress, so it goes ahead and sends `cancel`. Note: cancel request is buffered
// query processing resumes. Client receives the result successfully
// Client sents `very_important` query
// `cancel` signal passes through the network, and it causes the backend to kill `very_important` query

So if you ever use cancel API, the connection is not really usable until the cancel is processed somehow.

If you want to quickly shut down the connection, then you need to call Connection#abort(...) instead.

vlsi added a commit that referenced this issue Mar 4, 2020
…ording to protocol the server closes the connection once cancel is sent (connection reset exception is always thrown) (#1641)"

This reverts commit 23cce8a.
@mahmoudbahaa
Copy link
Contributor Author

@mahmoudbahaa mahmoudbahaa commented Mar 5, 2020

@vlsi ok i got your point. but since

The server will process this request and then close the connection. For security reasons, no direct reply is made to the cancel request message.

so the cancel request never receive any EOF and even if it does it doesn't mean the request was processed. to actually make sure the request was processed we need to wait for the connection to be closed. NOT for a reply to the cancel request. that is the correct indication of whether the cancel request was processed. so if you want to you can wait for isClosed()==true or something like that

@vlsi
Copy link
Member

@vlsi vlsi commented Mar 5, 2020

cancel request never receive any EOF and even if it does it doesn't mean the request was processed

backend code does the following:

  1. process cancel request
  2. close the socket

Apparently, if pgjdbc detects EOF, it means the backend has processed the request.

@mahmoudbahaa
Copy link
Contributor Author

@mahmoudbahaa mahmoudbahaa commented Mar 6, 2020

well correct me if I am wrong but it only means the SIGINT was sent to the backend but whether it was processed by backend (SIGINT just set cancelRequested = true in backend) so actual handling of cancel can still be done after the cancel is returned.

never mind I got what you mean since sending new query reset cancelRequested to false so we will be sure that cancelRequested will not affect the next query since this way we are sure that cancelRequested is set to true before the query is sent setting it back to false.

so yes I think you should go ahead with the revert

@mahmoudbahaa
Copy link
Contributor Author

@mahmoudbahaa mahmoudbahaa commented Mar 6, 2020

although adding a comment to the revert so everybody would understand why it is there and don't remove it by mistake it in the future would also be a good idea

vlsi added a commit that referenced this issue Mar 6, 2020
…ording to protocol the server closes the connection once cancel is sent (connection reset exception is always thrown) (#1641)"

This reverts commit 23cce8a.
davecramer pushed a commit to davecramer/pgjdbc that referenced this issue Jul 5, 2021
…o protocol the server closes the connection once cancel is sent (connection reset exception is always thrown) (pgjdbc#1641)

quote "The server will process this [cancel] request and then close the connection. For security reasons, no direct reply is made to the cancel request message." https://www.postgresql.org/docs/current/protocol-flow.html
davecramer pushed a commit to davecramer/pgjdbc that referenced this issue Jul 5, 2021
…ording to protocol the server closes the connection once cancel is sent (connection reset exception is always thrown) (pgjdbc#1641)"

This reverts commit 23cce8a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants