ZEO.asyncio: switch to async/await style
#195
Conversation
There was a problem hiding this comment.
You mention a slight loss of efficiency. It may be worthwhile to see if you can use zodbshootout to get a better picture: https://pypi.org/project/zodbshootout/
…nto asyncio3.5+
Co-authored-by: Jens Vagelpohl <[email protected]>
…nto asyncio3.5+
… support is dropped)
Failing
|
|
A description of installed packages for those GHA runners is at https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md. I don't see You can install additional OS packages from the GHA tests configuration. Edit Anecdotal evidence from my own testing machine: I usually just run |
|
P.S.: The test failures on both test runs (without the |
|
Jens Vagelpohl wrote at 2022-4-8 01:06 -0700:
...
Thank you for your help!
Analysing further in my local environment showed
something surprising:
* `libuv` is not loaded even when it is installed.
* apparently, `uvloop` defines the `uv` functions itself
even though I can not find those definitions in the
(distributed) pregenerated `loop.c`.
For example, if I ask `gdb` where I find
the `uv` function `uv_loop_init` it answers in `src/unix/loop.c, line 31`
(part of `loop.cpython-39-x86_64-linux-gnu.so`).
However, line 31 of `loop.c` contains `#include <stddef.h>` and
definitely not a definition for `uv_loop_init`.
I got the impression that the binary wheel was generated in
a very special way -- from a C source file that contained the
**source** of `libuv`.
This would mean that `uvloop` does not depend on a preinstalled `libuv`
-- at least not for `x86_64.linux-gnu`.
Likely, I have to look for other explanations why the uvloop tests
succeed in my environment and fail in others.
|
|
Exactly, after looking at the code and noticing that the |
|
@d-maurer, personally I do not have problems with dropping credentials support. However this pull-request is very large 26000 line diff doing many things at I had not looked closely yet, but please anyway find some feedback below. Hope
(emphasis mine) This change is a big source of potential concurrency problems in my view. The This change also significantly affects performance because latency of many Thus, I suggest not to make this change, or do it very carefully alone in a Second. The drop of Python2 support. I understand it is 2022, but still, even after it was deprecated, Python2 is PyPy is also not dropping Python2 support . Dropping Python2 support for ZODB was discussed some time ago several times already, e.g in zopefoundation/ZODB#312 In my personal view dropping this support brings only cosmetic benefit and is Kirill P.S. it is easy to extract small commits from a big change with /cc @perrinjerome, @arnaud-fontaine, @mgedmin, @jamadden, @dataflake |
The file
Please keep in mind:
You keep bringing up worries about stability. We're trying our best with very limited resources to move the project forward while keeping existing users in mind. Anything that has the potential to cause disruption will lead to a major release and anyone who finds they cannot use that new major release can stick with an older version. |
|
Kirill Smelkov wrote at 2022-4-8 13:27 -0700:
...
However this pull-request is very large 26000 line diff doing many things at
once and I strongly suggest splitting it in shorter, logically separate, steps.
Essentially, it does only 2 things:
* switch to `async/await` style (which forces to drop Python 2 support)
* get rid of `Random2`
The last point is responsible for most of the diff lines; but they
affect a single file (not otherwise affected by the changes).
I agree that the PR contains also cosmetics (splitting
the `ZeoBaseProtocol` into 2 explicit layers, rename some of
its methods, rename `Client` to `ClientIo`, profit from aync/await style
to streamline the interface between `ClientRunner` and `ClientIo`).
Part of this would not strictly be necessary but my primary
aim was to make the implementation more readable/understandable
and for this purpose, cosmetics can help.
It took us ~ 7 years to stabilize ZEO5 and it would be a huge pity to have it
destabilized again.
This effort has left as a side effect many tests checking for
concurrency problems. One of them has already detected
a hidden problem (and I added a test to check for it explicitly).
All those tests will remain and help to detect problems.
You probably understand that, but a reminder just in case
is that here we are dealing with such kind of software where concurrency
mistakes turn into data corruptions.
The PR entails a major rewrite: there is a risk to
introduce new problems or reintroduce old ones.
Our tests checking for those problems are not bad.
I promise to watch out for all test failures, especially isolated ones
(indicating race condition problems)
and to merge only if all observed problems have been understood/fixed.
I had not looked closely yet, but please anyway find some feedback below. Hope
it might be useful.
> It uses throughout standard asyncio Futures (**with scheduled callbacks**)
> instead of special futures (**with immediate callbacks**) at some places.
(emphasis mine)
This change is a big source of potential concurrency problems in my view. The
"call immediate rather than soon" type of future that ZEO5 uses has impact not
only on performance, but also on semantic. In particular with immediate call it
is guaranteed that if e.g. a message sending is complete the callback is
invoked immediately and another - semantically related message - could be sent
**right after the first one**. With switching to "invoke future callbacks
eventually" this property is lost. I see you already observed some problems
related to this change in
#193 (comment) and
I'm sure there are many places in current ZEO implementation that
undocumentedly rely on "invoke immediately" Fut behaviour.
You are right.
But the use of 3 different kinds of "Future"s (
`asyncio` futures for the connection process,
`concurrent` futures for the interface
between `ClientRunner` and `Client` (now `ClientIo`)
and proprietary futures for the server calls)
posed a major problem for me to understand the implementation.
Therefore, I wanted to get rid of this usage.
Now, the only visible type of futures are `asyncio` futures;
`concurrent` futures are still used under the hood - but
hidden behind `run_coroutine_threadsafe`.
This change also significantly affects performance because latency of many
operations will grow. There is a big difference in between being notified right
after an operation completes compared to being notified after an operation
completes and a full event loop cycle is over.
I am aware that performance drops **BUT** I doubt that it will
be significant: we speak here about the implementation of a network
protocol; the implemented RPC calls typically involve disk file access
(when ZEO uses `FileStorage`) -- the additional CPU load
should not be significant compared to the IO time.
Thus, I suggest not to make this change, or do it very carefully alone in a
step separated from everything else.
I will not do that: dropping the use of 3 different kinds
of futures is a major point of this PR.
Getting rid of the proprietary futures allows to consistently
use standard `asyncio` features to coordinate the coroutines.
It also facilitates the use of `async/await`:
the former implementation used its own coroutine executor
(called `future_generator`) designed for the proprietary futures.
It essentially executed
```
await f1
<continuation>
```
by putting `wrap(<continuation>)` into the callback of `f1`.
The task of `wrap` is to feed the result of `f1`
into `continuation`, then run `continuation` either until completion
or the next `await`. If it completes, the coroutine completes,
otherwise, the remaining continuation is wrapped and
put as callback of the awaited future.
Each time I looked at the implementation, I stumpled over
this corouting executor and asked myself "might it be responsible
for the problem I currently analyze?".
It never did but nevertheless, I wanted to get rid of it.
Initially, I believed that switching from the proprietary
coroutine executor to `async/await` requires the use
of `asyncio` futures. Likely, this is not the case:
`await` can be made work with (properly defined) proprietary
futures. The construct `await coroutine` will use the current
loop to turn the coroutine into a future, but it could
be transformed into `await proprietary_wrap(coroutine)`
to force the use of a proprietary future.
Nevertheless, I much prefer direct use of `await`
(and therefore the use of the futures associated with the loop).
Second. The drop of Python2 support.
I understand it is 2022, but still, even after it was deprecated, Python2 is
being used. For example we actually do currently use Python2 where I work,
because porting to Python3 takes a lot of effort and is slow to happen. I hope
it will eventually happen, but I do not see it coming in the near future.
The PR targets a new major ZEO version (6.x).
Package versions which still support Python 2 could stick to ZEO 5.x
(and not use ZEO 6).
...
In my personal view dropping this support brings only cosmetic benefit and is
outweighted by the fact that people, that still do use Python2, are being cut.
@dateflake reported recently that it becomes more and more
difficult to feed Zope 4 with current versions of auxiliary
packages - sometimes necessary to fix security problems -
because they have dropped Python 2 support. A security fix for
`waitress` (used as client facing IO component by `Zope`), available
only for Python 3.7+ has caused this report.
He suggested that, as a consequence, Python 2 support might need to get dropped
in the not so far future.
I remember that @dataflake has been one of those developers who
advocated not to drop Python 2 support without good reason.
I do not know whether the reasons behind my PR are good enough
to justify dropping Python 2 support.
I understand much better how the implementation works
with the "async/await" style introduced by the PR than
the former `future_generator`.
Not sure whether others agree and whether they think it is worth
to drop Python 2 and accept potential instabilities.
|
That is correct, I always asked to keep Python 2 support unless the extra work becomes too much of a burden. My main reason as a Zope release manager was Zope 4, which is supported on Python 2.7 and is still an officially supported Zope release. In the meantime, many third party dependencies we have no control over dropped Python 2 support left and right. Zope 4 is now stuck on outdated and unsupported dependencies, and already one of them is an unfixed security issue (the As a result, my opposition to dropping Python 2 support is now gone. There is no good reason to spend more time on a platform that we simply cannot support anymore due to reasons out of our own control.
My opinion: Only keep Python 2 support if you can reach the technical goal of your changes without bending over backwards to keep it compatible with Python 2. Users who need Python 2 or who are afraid of the changes have a viable workaround, they can stay on ZEO 5. |
…nto asyncio3.5+
…ed by an `asyncio` loop; this is prone to race conditions. Increase the time the observer thread gives the loop to reach a stable state to reduce the race condition risk
…tions) to "wait until inactive"
…to ensure that we do not store the same information twice into the cache; this may or may not be important) -------- kirr: for discussion Extracted from #195
|
Hello @d-maurer, I have finished reviewing your work. My review has the form of your original changes coming in adjusted form and divided in two parts:
That's it. If you are interested to check the overall difference in between your and mine The full list of changes is:
Please see the diff and individual patches for details. For completeness here are the other relevant deltas:
And there are a few things I did not picked up: a) moving cache access out of This change is questionable to me. In my understanding it is the invariant This change is self-contained and orthogonal to the rest. I've created #219 to discuss it further. b) removal of Those I can do corresponding PR if we decide that dropping c) d) removal of credentials support. Credentials are different from Hope it help. And last but not lest I want to say thank you for being open, listening to my Kirill (*) for the comparison I've merged asyncio3.5+ with master myself. This resulted in 751933ea. |
Yes, removal of all code that exists just to support running |
kirr:
Similarly to ZODB `python setup.py test` was not working properly for
some time already. For example running it today executes only 6 tests:
(py3.venv) kirr@deca:~/src/wendelin/z/ZEO$ python setup.py test
running test
WARNING: Testing via this command is deprecated and will be removed in a future version. Users looking for a generic test entry point independent of test runner are encouraged to use tox.
...
running build_ext
test_ZEO_DB_convenience_error (ZEO.tests.testZEO.Test_convenience_functions) ... ok
test_ZEO_DB_convenience_ok (ZEO.tests.testZEO.Test_convenience_functions) ... ok
test_ZEO_client_convenience (ZEO.tests.testZEO.Test_convenience_functions) ... ok
test_ZEO_connection_convenience_ok (ZEO.tests.testZEO.Test_convenience_functions) ... ok
test_ZEO_connection_convenience_value (ZEO.tests.testZEO.Test_convenience_functions) ... ok
test_work_with_multiprocessing (ZEO.tests.testZEO.MultiprocessingTests)
Client storage should work with multi-processing. ... ok
----------------------------------------------------------------------
Ran 6 tests in 0.070s
OK
Also @d-maurer and @dataflake say this way of running tests is being
removed throughout all Zopefoundation projects:
#217 (comment)
#195 (comment)
-> Let it go.
Cherry-picked from
zopefoundation/ZODB@3577a9c4 and adjusted a
bit for ZEO.
First major part of Dieter's work from #195 : Reimplement and streamline the ``asyncio`` part of the ``ClientStorage`` implementation: - switch from futures with explicit callbacks to `async/await`-like style - use standard ``asyncio`` features to implement timeouts - redesign the API of the class implementing the ZEO client protocol - significantly improve source documentation and tests - fix several concurrency bugs - add optional ``cython`` based optimization; it speeds up reads but slows down writes. To use it, install ``cython`` (and its dependencies) and run ``cythonize -i *.pyx`` in ``src/ZEO/asyncio``. NOTE: this works on both Python2 and Python3. /reviewed-by @d-maurer /reviewed-on #217
|
Dieter, thanks for feedback. I see. For the reference the differences in between #195 and #217+#218 are explained in #195 (comment). |
|
Anyway I've reopened this PR and retargeted it to master. |
@d-maurer notes at #218 (comment) : Why do we need this special __await__ logic? It is not present in #195 and nevertheless all uvloop tests passed. and he is right - in 7f574ec I missed to do the full removal of py2 support code from CoroutineExecutor: in py2 it was @coroutine def dothing1(): yield dothing2() this way the yield for dothing2 was working this way: - dothing2() called - it returns a coroutine object - yield yields that coroutine object, not objects that second coroutine object would yield by itself. This is the difference in beween yield and `yield from`. - CoroutineExecutor was noticing such yield and handling `yield coro()` with the same semantic as if it was `yield from coro()` by manually creating another AsyncTask. but on py3 await has semantic of `yield from` and so we do not need to keep all that `yield from` emulating code. 7f574ec dropped part of that, but did not drop special handling of `yield coro()` if coro is non-native custom coroutine function. Let that code go as well.
Second part of Dieter's work from #195 : - Switch to using `async/await` directly instead of `@coroutine/yield` - Drop support for Python 2.7, 3.5 and 3.6 /reviewed-by @d-maurer, @dataflake /reviewed-on #218
Drop experimental support for credentials object: the corresponding ``ClientStorage.__init__`` parameter ``credentials`` is retained but ignored. From now on ZEO supports authentication only via SSL certificates. Note that ZEO 5 never supported authenticating via ``username`` and ``password`` - support for such basic auth was dropped in 2016 before ZEO 5.0 was released. See c7f2138 for details. Extracted from #195 /reviewed-by @dataflake, @d-maurer /reviewed-on #220
|
The essential changes in the PR have been repackaged by @navytux (a requirement for his approval) and merged into master. Therefore, I close this PR. |
This PR tries to make the ZEO client interface implementation easier to understand; likely, the result is slightly less efficient than the current implementation.
The PR switches to standard
async/awaitstyle for theasynciopart of the client interface implementation. Therefore, it must drop Python 2 support.It uses throughout standard
asyncioFutures (with scheduled callbacks) instead of special futures (with immediate callbacks) at some places.It significantly extends (and at some places corrects) the source code documentation.
@navytux The PR drops "credentials" support. Maybe, this is a mistake. Two aspects made me drop it:
Thus, I definitely ignored the parameters. Only later, I noticed that they might still be usefull for the use case "ZEO 6 client + ZEO 4 server". If we want continued support for this use case, the "credentials" support can easily be restored.
The PR removes the testing dependency from
mockandRandom2.Random2was there to get the same randomization in Python 2 and Python 3. With Python 2 support dropped,Random2was no longer necessary. However, its replacement by Python 3'srandomcaused a huge diff fortest_cache.Note to reviewers: due to significant changes it is likely easier to look at the files directly rather than at the diffs.