Implementation refactor for Client-based interface #286

fantix · 2022-01-05T16:38:33Z

Review note: there is one merge commit "Partially revert ..." in this branch to reserve git history; use this link to see the merged diff.

Merge Connection with RawConnection
Fix tests to use client
Add previously-deleted tests
Complete timeout implementation and add more timeout tests (will be done in a new PR)
Drop edgedb.connect() and replace with a single-connection Client
Extract base class for Client, and further remove redundant code
Extract redundant code from transactions and only use _iter_coroutine() on top level
Review how pool is implemented, simplify if possible

This fixes the case when EdgeDB client is global and used in an ``asyncio.run`` call, the client uses a wrong loop.

Before this fix, the client allows multiple concurrent first connections. This wasn't an issue because the _working_addr is always the same among the concurrent first connections. However, it was not clear which address shall win the race and be used for upcoming connections when multiple addresses are used in the future. After this fix, all concurrent first connection attempts will be serialized, the address from the first successful connection will be used for following connections. Concurrency will be resumed as soon as the address is found, including the previously-serialized pending "first" connections.

Switch to using a single ``_connect_args`` dict to store all connect arguments, which is also consistent with ``con_utils.parse_connect_arguments()``.

Also added blocking pool (single-connection only), and fixed all tests to use client instead of connection.

Also close blocking client socket on disconnect, and fix tests

Also extracted most duplicate code, and dropped outdated code

In this case, concurrent close() and connect() are racing. If connect() wins, close() was releasing the connection by mistake.

elprans

Impressive work, Fantix! A net reduction of ~700 lines in the implementation despite the addition of a thread-safe concurrent blocking pool. I especially like the cleanup in the transaction implementation.

edgedb/base_client.py

edgedb/blocking_client.py

edgedb/base_client.py

edgedb/__init__.py

edgedb/blocking_client.py

tests/test_async_retry.py

1st1

Epic work, Fantix. I think this is ready to land.

@fantix

Changes ======= * Implementation refactor for Client-based interface (by @fantix in 33175f1 for #286)

Change was made in #286

fantix added 8 commits January 5, 2022 10:14

Don't acquire loop before the first acquire

d18d72b

This fixes the case when EdgeDB client is global and used in an ``asyncio.run`` call, the client uses a wrong loop.

Clean create_async_client() API and doc

5386427

Switch to using a single ``_connect_args`` dict to store all connect arguments, which is also consistent with ``con_utils.parse_connect_arguments()``.

Unnecessary _options assignment in _shallow_clone()?

529a23a

Drop _borrowed_for

5fdb357

Move _loop and _log_listeners away from _InnerConnection

f5cf6a0

Move _addrs, _config and _params away from _InnerConnection

ab0d87e

Move _codecs_registry and _query_cache away from _InnerConnection

ae4e42f

fantix force-pushed the refactor branch 4 times, most recently from 0efc2e4 to 93516b9 Compare January 7, 2022 15:05

fantix added 2 commits January 7, 2022 10:13

Drop _InnerConnection

9ef5970

Drop AsyncIOClient._acquire() and _release()

11cb497

fantix force-pushed the refactor branch from 93516b9 to 69d0564 Compare January 7, 2022 15:13

Merge Connection and RawConnection

053ab77

Also added blocking pool (single-connection only), and fixed all tests to use client instead of connection.

fantix force-pushed the refactor branch from 69d0564 to 053ab77 Compare January 7, 2022 19:14

fantix changed the title ~~[WIP] Refactor~~ [WIP] Implementation refactor for Client-based interface Jan 7, 2022

fantix added 2 commits January 10, 2022 10:22

Merge *_con.py and *_pool.py into *_client.py

b405dca

Extract redundant code into BaseClient and BaseImpl

5d4a0a5

fantix force-pushed the refactor branch from e202663 to 5d4a0a5 Compare January 10, 2022 16:10

fantix added 2 commits January 10, 2022 15:51

Partially revert 2bd27ae: test_pool.py

1f51604

Fix previously-deleted pool tests

97d212b

fantix force-pushed the refactor branch from 6e4e888 to 97d212b Compare January 11, 2022 15:24

Add __slots__ to Connection classes

447e585

fantix force-pushed the refactor branch 2 times, most recently from 88fe5ac to d5133d1 Compare January 11, 2022 22:05

Add pool to blocking client and drop single-conn

3f52217

fantix force-pushed the refactor branch from d5133d1 to 3f52217 Compare January 12, 2022 15:16

Add wait_for_disconnect to blocking client

d6b291d

Also close blocking client socket on disconnect, and fix tests

fantix mentioned this pull request Jan 13, 2022

getting ClientConnectionClosedError randomly #287

Open

fantix added 3 commits January 17, 2022 15:33

Refactor: use _iter_coroutine() on top level

5ae055f

Also extracted most duplicate code, and dropped outdated code

Fix a racing bug of using expire_connection()

386004c

In this case, concurrent close() and connect() are racing. If connect() wins, close() was releasing the connection by mistake.

Improve reliability of test_client_config_persistence

13bc2bd

fantix changed the title ~~[WIP] Implementation refactor for Client-based interface~~ Implementation refactor for Client-based interface Jan 18, 2022

fantix marked this pull request as ready for review January 18, 2022 21:13

fantix requested review from 1st1 and elprans January 18, 2022 22:09

elprans reviewed Jan 19, 2022

View reviewed changes

fantix added 4 commits January 19, 2022 10:15

Drop on_connect, on_acquire and on_release hooks

7269f8a

Drop public expire_connections()

4100f1c

Rename concurrnecy to max_concurrency

e6733ef

CRF: API updates

86fb29e

fantix force-pushed the refactor branch from 4f6fcd0 to 86fb29e Compare January 19, 2022 15:43

fantix mentioned this pull request Jan 20, 2022

Update test client to use the latest edgedb-python edgedb/edgedb#3355

Merged

1st1 reviewed Feb 1, 2022

View reviewed changes

tests/test_async_retry.py Outdated Show resolved Hide resolved

1st1 approved these changes Feb 1, 2022

View reviewed changes

Merge branch 'master' into refactor

4d39df0

fantix merged commit 33175f1 into edgedb:master Feb 2, 2022

elprans added a commit that referenced this pull request Feb 10, 2022

edgedb-python v0.22.0

deea761

Changes ======= * Implementation refactor for Client-based interface (by @fantix in 33175f1 for #286)

elprans mentioned this pull request Feb 10, 2022

edgedb-python v0.22.0 #291

Merged

msullivan added a commit that referenced this pull request Feb 18, 2022

Fix docs to say max_concurrency instead of concurrency

9363edb

Change was made in #286

msullivan mentioned this pull request Feb 18, 2022

Fix docs to say max_concurrency instead of concurrency #297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation refactor for Client-based interface #286

Implementation refactor for Client-based interface #286

fantix commented Jan 5, 2022 •

edited

Loading

elprans left a comment

1st1 left a comment

Implementation refactor for Client-based interface #286

Implementation refactor for Client-based interface #286

Conversation

fantix commented Jan 5, 2022 • edited Loading

elprans left a comment

Choose a reason for hiding this comment

1st1 left a comment

Choose a reason for hiding this comment

fantix commented Jan 5, 2022 •

edited

Loading