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

Add support for coroutine functions as listener callbacks #802

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

elprans
Copy link
Member

@elprans elprans commented Aug 9, 2021

The Connection.add_listener(), Connection.add_log_listener() and
Connection.add_termination_listener() now allow coroutine functions as
callbacks.

Fixes: #567.

@elprans elprans requested review from 1st1 and fantix August 9, 2021 20:52
@theo-brown
Copy link

Hi, I've also been working on this! Much less than you though, you've done a lot!

I've got a couple of questions, not necessarily criticisms just looking for clarification (I'm fairly new to async).

  1. Why did you use both create_task and call_soon, rather than just create_task? I thought they both run at the top of the next event loop.
  2. What is the perceived need for the _Callback class, beyond checking whether something is async?

@elprans
Copy link
Member Author

elprans commented Aug 9, 2021

  1. Why did you use both create_task and call_soon, rather than just create_task?

Because create_task works on coroutines and call_soon on regular callables.

  1. What is the perceived need for the _Callback class, beyond checking whether something is async?

To avoid calling inspect.iscoroutinefunction on every callback invocation, since this can be easily determined when the callback is set up.

@theo-brown
Copy link

Because create_task works on coroutines and call_soon on regular callables.

My mistake, I incorrectly thought that create_task could be used on both.

@theo-brown
Copy link

theo-brown commented Aug 9, 2021

Does this suggestion offer an improvement to your exception handling, or is there no impact?

  for cb in self._listeners[channel]:
      self._loop.create_task(self._call_listener(cb, ...)

async def _call_listener(cb, ...)
  try:
      if cb.is_async:
          await cb.cb(con_ref, pid, channel, payload)
      else:
          cb.cb(con_ref, pid, channel, payload)
    except:
        ....

@elprans
Copy link
Member Author

elprans commented Aug 9, 2021

Does this suggestion offer an improvement to your exception handling, or is there no impact?

The event loop already logs exceptions in tasks and callbacks fairly well, and, if anything, I think custom invocation of call_exception_handler there might be a vestige of old days of bad asyncio or something. That said, handling it this way for both types of callbacks is consistent at least.

The `Connection.add_listener()`, `Connection.add_log_listener()` and
`Connection.add_termination_listener()` now allow coroutine functions as
callbacks.

Fixes: #567.
@theo-brown
Copy link

Oh nice, that's so much tidier.
Would it be worth swapping the test queue from q3.put_nowait() to await q3.put()?

@elprans elprans merged commit 41da093 into master Aug 10, 2021
@elprans elprans deleted the async-callbacks branch August 10, 2021 00:16
elprans added a commit that referenced this pull request Aug 10, 2021
Changes
-------

* Drop support for Python 3.5 (#777)
  (by @and-semakin in da58cd2 for #777)

* Add support for Python 3.10 (#795)
  (by @elprans in abf5569 for #795)

* Add support for asynchronous iterables to copy_records_to_table() (#713)
  (by @elprans in 1d33ff6 for #713)

* Add support for coroutine functions as listener callbacks (#802)
  (by @elprans in 41da093 for #802)

* Add support for sslcert, sslkey and sslrootcert parameters to DSN (#768)
  (by @jdobes and @elprans in c674e86 for #768)

* Add copy_ wrappers to Pool (#661)
  (by @elprans in a6b0f28 for #661)

* Add issubset and issuperset methods to the Range type (#563)
  (by @kdorsel in de07d0a for #563)

Fixes
-----

* Break connection internal circular reference (#774)
  (by @fantix in d08a9b8 for #774)

* Make Server Version Extraction More Flexible (#778)
  (by @Natrinicle in d076169 for #778)
@elprans elprans mentioned this pull request Aug 10, 2021
elprans added a commit that referenced this pull request Aug 10, 2021
Changes
-------

* Drop support for Python 3.5 (#777)
  (by @and-semakin in da58cd2 for #777)

* Add support for Python 3.10 (#795)
  (by @elprans in abf5569 for #795)

* Add support for asynchronous iterables to copy_records_to_table() (#713)
  (by @elprans in 1d33ff6 for #713)

* Add support for coroutine functions as listener callbacks (#802)
  (by @elprans in 41da093 for #802)

* Add support for sslcert, sslkey and sslrootcert parameters to DSN (#768)
  (by @jdobes and @elprans in c674e86 for #768)

* Add copy_ wrappers to Pool (#661)
  (by @elprans in a6b0f28 for #661)

* Add issubset and issuperset methods to the Range type (#563)
  (by @kdorsel in de07d0a for #563)

Fixes
-----

* Break connection internal circular reference (#774)
  (by @fantix in d08a9b8 for #774)

* Make Server Version Extraction More Flexible (#778)
  (by @Natrinicle in d076169 for #778)
elprans added a commit that referenced this pull request Aug 10, 2021
Changes
-------

* Drop support for Python 3.5 (#777)
  (by @and-semakin in da58cd2 for #777)

* Add support for Python 3.10 (#795)
  (by @elprans in abf5569 for #795)

* Add support for asynchronous iterables to copy_records_to_table() (#713)
  (by @elprans in 1d33ff6 for #713)

* Add support for coroutine functions as listener callbacks (#802)
  (by @elprans in 41da093 for #802)

* Add support for sslcert, sslkey and sslrootcert parameters to DSN (#768)
  (by @jdobes and @elprans in c674e86 for #768)

* Add copy_ wrappers to Pool (#661)
  (by @elprans in a6b0f28 for #661)

* Add issubset and issuperset methods to the Range type (#563)
  (by @kdorsel in de07d0a for #563)

Fixes
-----

* Break connection internal circular reference (#774)
  (by @fantix in d08a9b8 for #774)

* Make Server Version Extraction More Flexible (#778)
  (by @Natrinicle in d076169 for #778)
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.

[Question] Asynchronous NOTIFY listener
3 participants