sysrepo/sysrepo-python

Notification handling not using asyncio

sjd-xlnx opened this issue · 3 comments

Firstly, thanks for your recent addition of the "notification" handling. Much appreciated.

However, I am having some difficulty with the async version of the subscribe_notification() function, which doesn't appear to work if the callback is "async", yet it does work if it is a regular function.

It seems that the code in subsciption.py, event_notif_tree_callback() is just calling the callback() function directly and not doing the usual check for whether it's async or not, c.f. "if is_async_func(callback):" which is used for other callbacks in subscription.py.

The following test code demonstrates the problem...

#!/usr/bin/env python3

import asyncio
import logging
import datetime
import sysrepo

async def notif_cb(xpath, notification_type, notification, timestamp, private_data):
    logging.debug("Received notification!")
    logging.debug(f'xpath={xpath}')
    logging.debug(f'notification_type={notification_type}')
    logging.debug(f'notification={notification}')
    dt = datetime.datetime.fromtimestamp(timestamp)
    logging.debug(f'notification={dt}')

async def test(session):
    notif_xpath = "/sysrepo-example:alarm-triggered"
    notif_dict = {"description": "An error occurred", "severity": 3}
    while True:
        await asyncio.sleep(5)
        logging.debug("Send notification")
        session.notification_send(notif_xpath, notif_dict)

def main():
    logging.basicConfig(format='%(levelname)s: %(message)s', level=logging.DEBUG)

    # Setup asyncio event-loop
    loop = asyncio.get_event_loop()

    try:
        with sysrepo.SysrepoConnection() as connection:
            with connection.start_session() as session:
                
                # Subscribe to notification
                session.subscribe_notification("sysrepo-example",
                                               "/sysrepo-example:alarm-triggered",
                                               notif_cb,
                                               asyncio_register=True)

                loop.run_until_complete(test(session))

    except sysrepo.SysrepoError as e:
        logging.error(f'SysrepoError: {e}')
    finally:
        loop.close()

if __name__ == "__main__":
    main()

It uses the "sysrepo-example.yang" and is supposed to send a notification every 5 seconds, which it does (I can see it in the netopeer2-cli). However, the callback notif_cb() never gets called. If you remove the "async" from "async def notif_cb()", then it works (regardless of the asyncio_register value) and the call-back gets called - but obviously this is now not an asynchronous call.

Hi @sjd-xlnx

this should be fixed by #19. Can you test again on your end?

I'll make a release with this patch asap.

Hi @rjarry ,
Thanks for looking at this so quickly!

I have tested the fix, and whilst it does fix the problem, there is one side-effect that is now evident.

In the fix you also added return values for the function. Well, I think the C function must be expecting nothing returned, since there is now a warning relating to this.

From cffi callback <function event_notif_tree_callback at 0x7f2273e500d0>:
Trying to convert the result back to C:
TypeError: callback with the return type 'void' must return None

If I remove the returns, i.e.

iff --git a/sysrepo/subscription.py b/sysrepo/subscription.py
index d5a9e3f..2a916f9 100755
--- a/sysrepo/subscription.py
+++ b/sysrepo/subscription.py
@@ -553,7 +553,7 @@ def event_notif_tree_callback(session, notif_type, notif, timestamp, priv):
         else:
             callback(xpath, notif_type, notif_dict, timestamp, private_data)
 
-        return lib.SR_ERR_OK
+        #return lib.SR_ERR_OK
 
     except BaseException as e:
         # ATTENTION: catch all exceptions!
@@ -562,4 +562,4 @@ def event_notif_tree_callback(session, notif_type, notif, timestamp, priv):
         LOG.exception("%r callback failed", locals().get("callback", priv))
         if isinstance(session, SysrepoSession) and isinstance(xpath, str):
             session.set_error(xpath, str(e))
-        return lib.SR_ERR_CALLBACK_FAILED
+        #return lib.SR_ERR_CALLBACK_FAILED

Then the TypeError message "TypeError: callback with the return type 'void' must return None" goes away. But, presumably you added these returns for a reason, so I'm not sure my "fix" is really correct - even though it seems to work.

Oops, my bad, I didn't look at the C function return value... Your fix is correct.