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

Ping first if conn is idle for too long #365

Merged
merged 3 commits into from
Oct 21, 2022
Merged

Ping first if conn is idle for too long #365

merged 3 commits into from
Oct 21, 2022

Conversation

fantix
Copy link
Member

@fantix fantix commented Sep 12, 2022

The blocking sockets don't have active connection_lost() events, if the user is trying to execute a non-read-only query over a disconnected connection, is_closed() cannot capture the disconnection and the retry loop wouldn't work. In this case, we are sending a SYNC before the query, so that we could capture the connection error and reconnect.

This is tailored to handle the server-side session_idle_timeout; for regular network interruptions within the session_idle_timeout, we don't want to sacrifice performance by inserting more frequent SYNCs just to avoid connection errors on non-read-only queries.

@fantix fantix requested a review from 1st1 September 12, 2022 16:36
@fantix
Copy link
Member Author

fantix commented Sep 12, 2022

This is tailored to handle the server-side session_idle_timeout; for regular network interruptions within the session_idle_timeout, we don't want to sacrifice performance by inserting more frequent SYNCs just to avoid connection errors on non-read-only queries.

Or maybe we do? We could also lazily select() for READ (and reconnect if necessary) every - say - (more than) 1 second per connection.


UPDATE: we don't, it's a rather marginal case and we cannot guarantee this doesn't happen at all.

edgedb/blocking_client.py Outdated Show resolved Hide resolved
@fantix fantix marked this pull request as ready for review September 13, 2022 23:09
@fantix
Copy link
Member Author

fantix commented Sep 14, 2022

Actually Elvis suggested over the meeting that, a proper fix should be that the server also sends a Terminate message before shutting down (idle) connections, so that the client won't have a vague state about the disconnection. I'll also do that, but this PR I think is still needed for EdgeDB 2.2 and lower.

@tailhook
Copy link
Contributor

tailhook commented Sep 15, 2022

  1. On unixes you can do just poll with POLLOUT. If OS thinks connection is down, it will return an error. I think you can achieve the same with select on windows.
  2. As Terminate is a subject to race condition, you have to handle it in every receive loop, which is quite inconvenient. And TCP RST packet sent by OS (which is caught by (1)) is almost as reliable as application specific Terminate packet.
  3. Sync is probably the only reliable way (it covers more than (1) and (2) combined), although costly. But that cost is probably not critical as it is only incurred when connection was idle for the long time.

Also we could probably skip Sync if we send Parse instead of cached query, but that is probably over-optimizing too niche case.

I would go with (1) or (3), the latter is in current PR. As I don't see a significant difference between (1) and (2), whereas (2) complicates implementations of all blocking and async clients.

@1st1
Copy link
Member

1st1 commented Oct 20, 2022

Sync is probably the only reliable way

Yeah.

The blocking sockets don't have active connection_lost() events, if the
user is trying to execute a non-read-only query over a disconnected
connection, is_closed() cannot capture the disconnection and the retry
loop wouldn't work. In this case, we are sending a SYNC before the
query, so that we could capture the connection error and reconnect.

This is tailored to handle the server-side `session_idle_timeout`; for
regular network interruptions within the `session_idle_timeout`, we
don't want to sacrifice performance by inserting more frequent SYNCs
just to avoid connection errors on non-read-only queries.
@fantix fantix merged commit 99cf78a into master Oct 21, 2022
@fantix fantix deleted the blocking-idle-ping branch October 21, 2022 00:55
fantix added a commit that referenced this pull request Oct 21, 2022
Changes
=======

* Implement dataclass for EdgeObject (#359)
  (by @fantix in dfb8c8b for #359)

* Redo edgedb basic types to inherit from builtin types (#366)
  (by @fantix in b11b991 for #366)

* Officially drop 3.6 support (#373)
  (by @msullivan in 7b76bc7 for #373)

* Support Python 3.11 (#375)
  (by @msullivan in 04b0da2 for #375)

* Codegen with the describe_query() API (#363)
  (by @fantix in 361221d for #363)

* Add codegen docs (#380)
  (by @colinhacks in 23dd42e for #380)

* Use typing_extension.Literal in codegen for Python 3.7
  (by @fantix in 6d0d6ab)

* Add __all__ to edgedb/__init__.py
  (by @fmoor in d3ef6d9)

Fixes
=====

* Improve duration parsing
  (by @fmoor in 241c80d)

* Tweak wording in query_single() error messages (#369)
  (by @msullivan in e24bb53 for #369)

* Fix flake tests on python3.7 (#371)
  (by @msullivan in 583e1cb for #371)

* Don't reject tuple arguments on the client side (#370)
  (by @msullivan in 09c950f for #370)

* Correct edgedb.Client.close() timeout behavior
  (by @fantix in 33a912c)

* Ping first if conn is idle for too long (#365)
  (by @fantix in 99cf78a for #365)

Deprecations
============

* Deprecate object links and simplify link property access (#379)
  (by @fantix in 2c5dcc7 for #379)
@fantix fantix mentioned this pull request Oct 21, 2022
fantix added a commit that referenced this pull request Oct 21, 2022
Changes
=======

* Implement dataclass for EdgeObject (#359)
  (by @fantix in dfb8c8b for #359)

* Redo edgedb basic types to inherit from builtin types (#366)
  (by @fantix in b11b991 for #366)

* Officially drop 3.6 support (#373)
  (by @msullivan in 7b76bc7 for #373)

* Support Python 3.11 (#375)
  (by @msullivan in 04b0da2 for #375)

* Codegen with the describe_query() API (#363)
  (by @fantix in 361221d for #363)

* Add codegen docs (#380)
  (by @colinhacks in 23dd42e for #380)

* Use typing_extension.Literal in codegen for Python 3.7
  (by @fantix in 6d0d6ab)

* Add __all__ to edgedb/__init__.py
  (by @fmoor in d3ef6d9)

Fixes
=====

* Improve duration parsing
  (by @fmoor in 241c80d)

* Tweak wording in query_single() error messages (#369)
  (by @msullivan in e24bb53 for #369)

* Fix flake tests on python3.7 (#371)
  (by @msullivan in 583e1cb for #371)

* Don't reject tuple arguments on the client side (#370)
  (by @msullivan in 09c950f for #370)

* Correct edgedb.Client.close() timeout behavior
  (by @fantix in 33a912c)

* Ping first if conn is idle for too long (#365)
  (by @fantix in 99cf78a for #365)

* Use @-prefixed keys in object codec for link properties (#384)
  (by @fantix in 68480f9 for #377)

Deprecations
============

* Deprecate object links and simplify link property access (#379)
  (by @fantix in 2c5dcc7 for #379)
@fantix fantix mentioned this pull request Oct 21, 2022
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