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

Cancel http read only queries if client socket goes away #4213

Merged
merged 3 commits into from
Feb 4, 2019
Merged

Cancel http read only queries if client socket goes away #4213

merged 3 commits into from
Feb 4, 2019

Conversation

nvartolomei
Copy link
Contributor

@nvartolomei nvartolomei commented Feb 1, 2019

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

For changelog. Remove if this is non-significant change.

Category (leave one):

  • Improvement

Short description (up to few sentences):

Cancel http read only queries if client socket goes away

Detailed description (optional):

To check the socket status, try to read one byte from socket in a
non-blocking way:
0 - client closed the connection
>= 1 - client send more data, we are ignoring this case for now
timeout - normal case, client is waiting for response
... - socket broken?

Dirty, but should do the job. Limiting to readonly queries as I don't
want to mess with alter queries / insert select and others.

Closes #1403

@nvartolomei
Copy link
Contributor Author

Hopefully poco doesn’t support pipelining, this may break things *.

@nvartolomei
Copy link
Contributor Author

Can Socket#poll be used for this without abusing read and making too many assumptions about how Poco handles sockets?

To check the socket status, try to read one byte from socket in a
non-blocking way:
      0 - client closed the connection
   >= 1 - client send more data, we are ignoring this case for now
timeout - normal case, client is waiting for response
    ... - socket broken?

Dirty, but should do the job. Limiting to readonly queries as I don't
want to mess with alter queries / insert select and others.
@alexey-milovidov
Copy link
Member

Let's write a test with cancellation of infinite query like SELECT count() FROM system.numbers.

@nvartolomei
Copy link
Contributor Author

@alexey-milovidov the test is in, let me know if it works.

re readonly guard, I didn't want to alter the behaviour for pending inserts, alters or any other similar queries. Although this better be handled in other places (it might be actually), I wanted to be extra cautious. (also, initially it was implemented without _PEEK flag).

re poll, I tried if (socket->poll(Poco::Timespan(), SelectMode::ERROR)) cancelQuery() but it does not pass the test on macOS, haven't checked other OS. Maybe this is not the correct way of doing it, I'm not very familiar with these things.

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.

HTTP interface: kill queries after disconnect.
3 participants