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

Remove the synchronous RPC mode #315

Merged
merged 21 commits into from
Jun 12, 2023
Merged

Conversation

LasseBlaauwbroek
Copy link
Contributor

@LasseBlaauwbroek LasseBlaauwbroek commented Jun 9, 2023

Done in this PR:

  • Removed the possibility of running RPC without asyncio
  • All capability methods now have to be async. Calling methods can no longer be done using .wait(). Chaining can no longer be done using then()
  • Removed VoidPromise and most of Promise. Only RemotePromise remains as a major class, for pipelining purposes.
  • Removed the possibility of starting a server/client through a raw file descriptor. Only AsyncIoStreams are allowed now.
  • Moved the tests to use asyncio, and removed any tests and examples that were specifically concerned with synchronous mode.
  • Fixed a bunch of failing tests. Everything seems to run more stable now.
  • Removed a lot of dead code.
  • Some miscellaneous fixes.

@LasseBlaauwbroek
Copy link
Contributor Author

Note that I recommend reviewing commit-by-commit. They should be reasonably self-contained.

@LasseBlaauwbroek LasseBlaauwbroek changed the title Remove the synchronous mode Remove the synchronous RPC mode Jun 10, 2023
All of these tests also exist in test_capability.py. The only difference is the
way the .capnp file is loaded. But that could be tested with much less code.
@LasseBlaauwbroek
Copy link
Contributor Author

LasseBlaauwbroek commented Jun 11, 2023

Additional cleanups:

  • Deleted test_capability_old.py. It's tests also exist in test_capability.py, except that the old version uses explicit .capnp file loading through a fixture. But this is already tested in load_test.py.
  • Remove the .capnp file loading fixture in test_capability_context.py in favor of the approach in test_capapbility.py.
  • Remove the option of creating a capability server through the _new_server option. (This seems very ugly, and having two ways to do the same thing means more code-paths to test.)

Using a fixture makes things more complicated. If we want to test explicit
capnp.load() functionality, we can do that separately
Inheritance is required now to create a server
@haata haata merged commit b439993 into capnproto:master Jun 12, 2023
12 checks passed
@haata
Copy link
Collaborator

haata commented Jun 12, 2023

Thanks!

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

2 participants