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.

Sign up for GitHub

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

Jump to bottom

Fix retrying_transaction() on network errors #222

Merged
merged 1 commit into from
Sep 14, 2021
Merged

Fix retrying_transaction() on network errors #222

merged 1 commit into from
Sep 14, 2021

Conversation

fmoor
Copy link
Member

@fmoor fmoor commented Aug 14, 2021 •
edited
Loading

Errors from blocking socket operations are wrapped with EdgeDB errors.

@elprans elprans requested review from 1st1 and fantix August 19, 2021 22:14
@1st1
Copy link
Member

1st1 commented Aug 19, 2021

@fantix please review, you're working with this code right now so you definitely know its quirks better than I

fantix
fantix reviewed Aug 20, 2021
View reviewed changes
Copy link
Member

@fantix fantix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me in general, have a question though.

edgedb/retry.py Outdated Show resolved Hide resolved
edgedb/retry.py Outdated Show resolved Hide resolved
@fmoor fmoor force-pushed the retry-neterr branch 3 times, most recently from 02b065a to f5c2d0c Compare August 20, 2021 22:46
fantix added a commit to fantix/edgedb-python that referenced this pull request Aug 24, 2021
@fantix
Fix broken pool connection cleanup
f4cad66 
When a connection is broken (not actively closed by the user), that
connection holder in the pool is not cleaned, leading to issues like
creating zombie connections in the pool or pool.aclose() hangs forever.

To test retrying_transaction(), connection errors from
wait_for_message() is wrapped into a retryable EdgeDB client error type.
This is partially fixing the same issue in edgedb#222, but the latter should
aim for a more complete solution.
@fantix fantix mentioned this pull request Aug 24, 2021
Merged
fantix
fantix requested changes Aug 24, 2021
View reviewed changes
Copy link
Member

@fantix fantix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per discussion, I think we should repurpose this PR to wrap all eligible ConnectionError raised from edgedb-python into one of the retryable errors:

  • ClientConnectionFailedTemporarilyError
  • ClientConnectionTimeoutError
  • ClientConnectionClosedError

So that we don't even have to change the logic in __aexit__() of retry.py - the connection will try to reconnect automatically on the next acquisition and query. And for a pool we're using LIFO queue, so it's almost always reconnected on the next retry.

Please see also #230

fantix added a commit to fantix/edgedb-python that referenced this pull request Aug 24, 2021
@fantix
Fix broken pool connection cleanup
46b8e43 
When a connection is broken (not actively closed by the user), that
connection holder in the pool is not cleaned, leading to issues like
creating zombie connections in the pool or pool.aclose() hangs forever.

To test retrying_transaction(), connection errors from
wait_for_message() is wrapped into a retryable EdgeDB client error type.
This is partially fixing the same issue in edgedb#222, but the latter should
aim for a more complete solution.
fantix added a commit that referenced this pull request Aug 24, 2021
@fantix
Fix broken pool connection cleanup
be44959 
When a connection is broken (not actively closed by the user), that
connection holder in the pool is not cleaned, leading to issues like
creating zombie connections in the pool or pool.aclose() hangs forever.

To test retrying_transaction(), connection errors from
wait_for_message() is wrapped into a retryable EdgeDB client error type.
This is partially fixing the same issue in #222, but the latter should
aim for a more complete solution.
@fmoor fmoor requested a review from fantix September 13, 2021 17:14
@fmoor fmoor changed the title Retry retrying_transaction() on network errors Fix retrying_transaction() on network errors Sep 13, 2021
fantix
fantix requested changes Sep 14, 2021
View reviewed changes
Copy link
Member

@fantix fantix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the blocking transaction retries!

  • In protocol.pyx, there is still one usage of ConnectionError in SansIOProtocol.terminate() which should be changed.
  • Some auto test would be great, but this can be a separate PR.

edgedb/retry.py Outdated Show resolved Hide resolved
edgedb/retry.py Outdated Show resolved Hide resolved
@fmoor
Fix retrying_transaction() on network errors
42f3b5a 
Errors from blocking socket operations are wrapped with edgedb errors.
@fmoor fmoor merged commit cc001e6 into master Sep 14, 2021
@fmoor fmoor deleted the retry-neterr branch September 14, 2021 18:41
elprans added a commit that referenced this pull request Nov 12, 2021
@elprans
edgedb-python v0.18.0
7f96277 
Changes
=======

* support protocol version 0.12
  (by @fmoor in 5ce52ed)

* Update to protocol version 0.13
  (by @1st1 in c690250, 8by @fmoor in 5ce52ed, by @jaclarke in a5a6218 for #253)

* Lookup edgedb.toml recursively in parent directories
  (by @fmoor in d9a12b8 for #245)

* Update connection parameter resolution
  (by @jaclarke in 085f574 for #241)

* Implement EDGEDB_CLIENT_SECURITY
  (by @fmoor in ac46c37)

* Add optional/required `query_single*` methods + rename `retrying_transaction` + update pool to `create_client` API
  (by @jaclarke in f2ae0d0 for #249)

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

* Deprecate Pool.acquire() and Pool.release()
  (by @fmoor in 2d501e9 for #217)

* Rename tls_verify_hostname to tls_security
  (by @fmoor in 2086b86)

Fixes
=====

* Do not attempt to del transport in connection_lost if it has been already
  (by @elprans in c719e79 for #215)

* Retry if start() raises a retryable error (#228)
  (by @fantix in ffaae01 for #228)

* Fix broken pool connection cleanup
  (by @fantix in be44959 for #222)

* Fix deprecated usage of SSLContext (#231)
  (by @elprans in f73f999 for #231)

* Bugfix: _borrowed_for is now set entering a transaction (#233)
  (by @fantix in bf763d1 for #233)

* Send zero arguments as zero-length bytes in proto 0.12 (#238)
  (by @tailhook in f51dd51 for #238)

* Fix retrying_transaction() on network errors
  (by @fmoor in cc001e6)

* Fix connection and retry options on AsyncIOPool (#237)
  (by @tailhook in 44e279f for #237)

* Auto retry read-only queries outside transactions (#243)
  (by @fmoor in 76bb586 for #243)

* Fix `credentials_file` argument typo on `async_connect` (#252)
  (by @mkniewallner in b21b70a for #252)
elprans added a commit that referenced this pull request Nov 12, 2021
@elprans
edgedb-python v0.18.0
2272824 
Changes
=======

* support protocol version 0.12
  (by @fmoor in 5ce52ed)

* Update to protocol version 0.13
  (by @1st1 in c690250, 8by @fmoor in 5ce52ed, by @jaclarke in a5a6218 for #253)

* Lookup `edgedb.toml` recursively in parent directories
  (by @fmoor in d9a12b8 for #245)

* Update connection parameter resolution
  (by @jaclarke in 085f574 for #241)

* Implement `EDGEDB_CLIENT_SECURITY`
  (by @fmoor in ac46c37)

* Add optional/required `query_single*` methods + rename `retrying_transaction` + update pool to `create_client` API
  (by @jaclarke in f2ae0d0 for #249)

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

* Deprecate `Pool.acquire()` and `Pool.release()`
  (by @fmoor in 2d501e9 for #217)

* Rename `tls_verify_hostname` to `tls_security`
  (by @fmoor in 2086b86)

Fixes
=====

* Do not attempt to del transport in `connection_lost` if it has been already
  (by @elprans in c719e79 for #215)

* Retry if `start()` raises a retryable error (#228)
  (by @fantix in ffaae01 for #228)

* Fix broken pool connection cleanup
  (by @fantix in be44959 for #222)

* Fix deprecated usage of `SSLContext` (#231)
  (by @elprans in f73f999 for #231)

* Bugfix: `_borrowed_for` is now set entering a transaction (#233)
  (by @fantix in bf763d1 for #233)

* Send zero arguments as zero-length bytes in proto 0.12 (#238)
  (by @tailhook in f51dd51 for #238)

* Fix `retrying_transaction()` on network errors
  (by @fmoor in cc001e6)

* Fix connection and retry options on `AsyncIOPool` (#237)
  (by @tailhook in 44e279f for #237)

* Auto retry read-only queries outside transactions (#243)
  (by @fmoor in 76bb586 for #243)

* Fix `credentials_file` argument typo on `async_connect` (#252)
  (by @mkniewallner in b21b70a for #252)
@elprans elprans mentioned this pull request Nov 12, 2021
Merged
elprans added a commit that referenced this pull request Nov 12, 2021
@elprans
edgedb-python v0.18.0
a854632 
Changes
=======

* support protocol version 0.12
  (by @fmoor in 5ce52ed)

* Update to protocol version 0.13
  (by @1st1 in c690250, 8by @fmoor in 5ce52ed, by @jaclarke in a5a6218 for #253)

* Lookup `edgedb.toml` recursively in parent directories
  (by @fmoor in d9a12b8 for #245)

* Update connection parameter resolution
  (by @jaclarke in 085f574 for #241)

* Implement `EDGEDB_CLIENT_SECURITY`
  (by @fmoor in ac46c37)

* Add optional/required `query_single*` methods + rename `retrying_transaction` + update pool to `create_client` API
  (by @jaclarke in f2ae0d0 for #249)

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

* Deprecate `Pool.acquire()` and `Pool.release()`
  (by @fmoor in 2d501e9 for #217)

* Rename `tls_verify_hostname` to `tls_security`
  (by @fmoor in 2086b86)

Fixes
=====

* Do not attempt to del transport in `connection_lost` if it has been already
  (by @elprans in c719e79 for #215)

* Retry if `start()` raises a retryable error (#228)
  (by @fantix in ffaae01 for #228)

* Fix broken pool connection cleanup
  (by @fantix in be44959 for #222)

* Fix deprecated usage of `SSLContext` (#231)
  (by @elprans in f73f999 for #231)

* Bugfix: `_borrowed_for` is now set entering a transaction (#233)
  (by @fantix in bf763d1 for #233)

* Send zero arguments as zero-length bytes in proto 0.12 (#238)
  (by @tailhook in f51dd51 for #238)

* Fix `retrying_transaction()` on network errors
  (by @fmoor in cc001e6)

* Fix connection and retry options on `AsyncIOPool` (#237)
  (by @tailhook in 44e279f for #237)

* Auto retry read-only queries outside transactions (#243)
  (by @fmoor in 76bb586 for #243)

* Fix `credentials_file` argument typo on `async_connect` (#252)
  (by @mkniewallner in b21b70a for #252)
elprans added a commit that referenced this pull request Nov 12, 2021
@elprans
edgedb-python v0.18.0
36efb5f 
Changes
=======

* support protocol version 0.12
  (by @fmoor in 5ce52ed)

* Update to protocol version 0.13
  (by @1st1 in c690250, 8by @fmoor in 5ce52ed, by @jaclarke in a5a6218 for #253)

* Lookup `edgedb.toml` recursively in parent directories
  (by @fmoor in d9a12b8 for #245)

* Update connection parameter resolution
  (by @jaclarke in 085f574 for #241)

* Implement `EDGEDB_CLIENT_SECURITY`
  (by @fmoor in ac46c37)

* Add optional/required `query_single*` methods + rename `retrying_transaction` + update pool to `create_client` API
  (by @jaclarke in f2ae0d0 for #249)

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

* Deprecate `Pool.acquire()` and `Pool.release()`
  (by @fmoor in 2d501e9 for #217)

* Rename `tls_verify_hostname` to `tls_security`
  (by @fmoor in 2086b86)

Fixes
=====

* Do not attempt to del transport in `connection_lost` if it has been already
  (by @elprans in c719e79 for #215)

* Retry if `start()` raises a retryable error (#228)
  (by @fantix in ffaae01 for #228)

* Fix broken pool connection cleanup
  (by @fantix in be44959 for #222)

* Fix deprecated usage of `SSLContext` (#231)
  (by @elprans in f73f999 for #231)

* Bugfix: `_borrowed_for` is now set entering a transaction (#233)
  (by @fantix in bf763d1 for #233)

* Send zero arguments as zero-length bytes in proto 0.12 (#238)
  (by @tailhook in f51dd51 for #238)

* Fix `retrying_transaction()` on network errors
  (by @fmoor in cc001e6)

* Fix connection and retry options on `AsyncIOPool` (#237)
  (by @tailhook in 44e279f for #237)

* Auto retry read-only queries outside transactions (#243)
  (by @fmoor in 76bb586 for #243)

* Fix `credentials_file` argument typo on `async_connect` (#252)
  (by @mkniewallner in b21b70a for #252)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elprans elprans elprans left review comments

@fantix fantix fantix approved these changes

@1st1 1st1 Awaiting requested review from 1st1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fmoor @1st1 @elprans @fantix