Eyepea / aiosip

SIP support for AsyncIO (DEPRECATED)
Apache License 2.0
82 stars 41 forks source link

Dialog redesign proposal #69

Closed vodik closed 6 years ago

vodik commented 6 years ago

So heads up, sorry if this is a bit rambly or poorly structured, I've been writing this while distracted with other stuff at work. Hopefully its coherent enough... If not, feel free to ask questions.


So I've spent the last two or three days playing with aiosip and trying to cleanup the API, trying to look at look at how to improve client and server API, and tackle issues like #63 and #59.

I'm playing with some API changes and I really think we should now outright remove create_dialog and the dialog router from the public API. On top of, imho, more correct client code, it leads to cleaner and easier to read code and easier to maintain tests as well.

I'm currently working on a hard fork of aiosip to test these ideas without having to worry about breaking the proxy support (don't worry, I'll fold everything back if it works out), but I suspect it will also help cleanup that code as well:

Dialogs are established with a SIP transaction

I know I've mentioned this one in the past, so I'll keep it short (see #59 for more details).

I think create_dialog is the wrong API to expose. Instead of doing:

dialog = peer.create_dialog(...)
response = dialog.subscribe(...)

We should instead be exposing something like:

dialog = peer.subscribe(...)

We could either raise exceptions or stash status_code/status_message on the dialog for failures here. If a failure does happen, we can automatically cleanup and remove the dialog. Technically, if a SUBSCRIBE or REGISTER fail, as I understand it, it should be retried inside another dialog.

Inside a dialog, messages should be handled linearly

I want to get rid of the in-dialog router. I don't think callbacks are appropriate here. Instead of providing a router, iterate on (or call recv() on) the dialog directly.

This forces all messages in a dialog to be handled, and forces the user to think about the order they arrive in. Technically not impossible with callbacks, but callbacks obfuscate this.

dialog = peer.subscribe(...)
async for msg in dialog:
    print(msg)
    msg.reply(status_code=200)

Order is important, and so it handling all messages.

It also means that a single coroutine can be responsible for a single dialog, which is also a nice design perk to have. It keeps call state well localized and easy to inspect, instead of spread across multiple callbacks that might have been triggered in an undeterministic order (not because aiosip is undeterministic, because the other user agent is).

Better server side handling of dialogs

Our subscribe example server currently looks somewhat similar to this:

def on_subscribe(msg, dialog):
    msg.reply(status_code=200)
    async for event in some_event_source():
        dialog.send("NOTIFY", payload=event)

But what happens when the client refreshes their subscription? What if we care if a subscription has expired or not?

Remember, the re-subscription comes on the same dialog as the original subscription, but with a bumped CSeq number. Currently the on_subscribe callback is fired a second time.

Now while this can be worked around with some more bookkeeping, we probably don't want this callback to trigger a second time for the same endpoint. Its a potential trap where we'll end up sending the same NOTIFY messages twice now to the same endpoint instead (I hit this when building testing tools).

But the new dialog API could be leveraged server side too - a more complicated and complete example:

def on_subscribe(msg, dialog):
    msg.reply(status_code=200)

    async def reader():
        async for new_msg in dialog:
            if new_message.method != 'SUBSCRIBE':
                # ... unknown message
                continue

            # ... resubscribe

    async def writer():
        async for event in some_event_source():
            # Make sure the subscription hasn't expired
            if is_expired():
                break

            dialog.send("NOTIFY", payload=event)

    asyncio.gather(reader(), writer())

We got a better grip on when a subscription is valid, when an unsubscription happens, etc.

It also moves to having a single coroutine responsible for a single dialog, which I already stated I think is desirable.

The router is still useful, as we still need to implement callback for inbound traffic that will establish new dialogs, but it should not be reused inside dialogs.

Leads to better high level abstractions

Registrations, subscriptions, etc have to be maintained. But we can now expose them with a higher level APIs that do the right thing - e.g. send messages automatically to maintain the registration and automatically unregister when necessary.

async with peer.registration(...):
    # All code from here on in will be run with the registration in
    # place, and automatically refreshed as necessary

or

async with peer.subscription(...) as subscription:
    async for msg in subscription:
        # handle incoming notify messages
ovv commented 6 years ago

This all looks very very good :+1:

We are focused on other things at the moment but I really like where all this is going. We use fixed version of aiosip when we need it so I'm not too worried about things breaking. We should put something in the readme that all API are provisional.

Feel free to merge what you think is ready if you don't get any feedback after a bit :)