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

Asyncio-based ZEO client and server #21

Closed
wants to merge 153 commits into from
Closed

Asyncio-based ZEO client and server #21

wants to merge 153 commits into from

Conversation

jimfulton
Copy link
Member

@jimfulton jimfulton commented May 31, 2016

The main things in this change:

  • Change from one asynchronous networking framework (asyncore) to
    another (asyncio).
  • Make connection logic much simpler.
  • Simplify client threading model, eliminating many locks
  • Reduce extra levels of indirection that made the code harder to reason about.

How to approach reviewing this?

I would recommend starting with ZEO.asyncio, noting that the cache is
now managed by the networking thread and doesn't require locks, then
look at the changes to ClientStorage, StorageServer, and ZEO.runzeo
and ZEO.tests.forker. I would focus on one side at a time, client or server.

Ask questions in the PR or on IRC.

If you're feeling especially thorough, you could review the test changes.

Jim Fulton and others added 30 commits December 13, 2015 16:18
Much left to do, but this is a nice little spike.
- Moved cache into async thread to avoid lots of locking.

- Setup delegation to storage.

- Provide thread wrapper that runs the async protocol in a thread.
Also got rid of the adapter machinery. It didn't buy enough to justify
the wrapping.
Renamed connection_timeout to connect_poll and use it when reconnecting.

Optimized hanfline of first messagem, as we did in zrpc, because it
only occurs once. :)

More/better comments.

Move loop argument to front of constructor arguments.

Added close/close_threadsafe to wait for connection on close.

Added is_connected.

Added new_addr.
- Fixed tpc_finish:

  - Use tid from server to update cache.

  - Accept and call callback function.

- Implemented flow control

- Added connection/disconnection notification (to client storage).

- implemented get_peername.

- implemented is_read_only

- renamed callAsync to async (death to Camels!)
- Issue with notify_connected, ClientStorage wants to make requests in
  response to being notified.  This is problematic because
  synchronsouse calls cause deadlock in this situation as do
  asyncronous calls done in a multi-threaded fashion.

  - Call get_info from io thread during startup, because
    notify_connected wants it.

  - Added an same-thread asyncronous API.

  - Added comment warning of this issue.

  - Added a little more logging.

- fixed an ordering issue when protocol is disconnected. It should
  notify the client before it cleans up it's futures to prevent
  getting more.

- Expose protocol_version to client so it can adjust it's behavior to
  the .

- More logging
Many tests passing. Quite a few still failing.
- testZEO tests now pass

- async tests now pass again

  Probably need to write more async tests to reflect changes.
  (Or maybe the ZEO tests that drove tem are enough.)

- dropped heartbeat tests, which were insane. Will add simpler test
  when I add heartbeats to the async implementation.
Don't test obsolete method ``invalidate``. It's not used anymore in
master and not present on this (asyncio) branch.

Removed the reuable tests because transaction buffers are no-longer reused.
Because it's only accessed by the asyncio thread.
…ientDisconected errors.

To make matters worse, when the other site closes a connection, None
is passed to protocol connection_lost methods, which was fouling up
sornstream error detection.
We were sending lastTransaction requests while register requests were
in-flight.  If register failed, then the lastTransaction request was
invalid, causing the connection to be closed. :(

When we update the server, we'll have register return lastTransaction
and probably info, since the client wants that information on connect.
And make waiting for reconnect a little easier.
Need to check for None as data value.
Jim Fulton and others added 29 commits June 29, 2016 13:31
I develop on a mac (for better or worse) so I know that mac tests are being run.
It's a shame that Tox is unable (or unwilling) to get test
dependencies from package meta data. :(
…he entry

This used to happen via the server calling serialnos, but ZEO 5
servers no longer do this.  For the reason for invalidating the cache
entry, see the new comment.
There are some cases where we want to touch the cache outside of the
I/O thread. Including:

- ClientStorage wants to to invalidata cache entries if it gets a
  conflict error in voting.

- loadBefore can probably be optimized by checking the cache in the
  client thread. (This will be safe for loadBefore, because the intent
  in practice is never to load current data.
ZEO's vote error handling was extremely and weirdly complicated.
There's a hysterical explanation: Originally, the ZEO protocol was
synchronous and mirrored the storage API. That was too slow, so store
was made asynchronous.  But then there was no way to return new
serials, so we added a serialnos client API that the server called
during TPC. This was used both to serials and errors.  Later, tpc_vote
was added, which is a synchronous method. That would have been an
execllent opportunity to fix this, but noooooooooo.

I'd like the server vote logic to get simpler, and we also want to get rid
of serialnos, as it makes it hard to commit multiple transactions at
once on the client.  We can't get rid of serialnos now, because the
client needs to work with old servers.

Of course, nothing is easy. There was a special edge case where if the
client saw an unhandled conflict error, it would invalidate it's cache
for that object, allowing it to eventually recover when caches had
missed invalidations. This required a change to ClientStorage, which
meant that the server wouldn't work with older ZEO versions, which
meant that the server could only support protocol Z5, which in tern
affected protocol-negotiation tests....
As it depends on getting serials from stpre or vote
Conflicts:
	src/ZEO/ClientStorage.py
This is a nasty work around for: https://bugs.python.org/issue27456

It can't always be applied.  For example, it can't be applied to Linux
SSL sockets using the asyncio-provided loop.
Conflicts:
	src/ZEO/ClientStorage.py
I'd missed that you could get a transport's socket using get_extra_info.
Add SSL support +

It's sad no reviewer, but I suppose it can be reviewed even after a merge.
…ct-resolution

Conflicts:
	src/ZEO/ClientStorage.py
	src/ZEO/StorageServer.py
I'm losing my love of stubs. :(
@jimfulton jimfulton closed this Jul 7, 2016
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