Eyepea / aiosip

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

Transactions loose ACK messages #59

Closed vodik closed 6 years ago

vodik commented 6 years ago

Because of the way transactions are tracked by method + cseq, transactions never get their ACK messages forwarded to them:

https://github.com/Eyepea/aiosip/blob/a26d3acae1631c75648cbff4ae1d7473a600d711/aiosip/dialog.py#L56-L61

I had to put in a hack to rewrite ACK to INVITE to make it work.

vodik commented 6 years ago

As I understand it, the dialog isn't really considered ready until an INVITE-like transaction completes. So maybe it makes sense to separate the two phases from each other...

vodik commented 6 years ago

How is this for a proposal: instead of creating a dialog directly, we tie it directly to the transaction:

async with peer.invite() as dialog:  # instead of create dialog
    assert dialog.status == PENDING
    async for msg in dialog:
       ... # handle 100/180/200

    await dialog.start()  # send ack
    assert dialog.status == IN_CALL
    # ... use the 

assert dialog.status == CLOSED
ovv commented 6 years ago

I'm not sure I understand what you mean by:

As I understand it, the dialog isn't really considered ready until an INVITE-like transaction completes. So maybe it makes sense to separate the two phases from each other...

As I see it we have two choices:

Your proposal look interesting but I'm afraid tying transactions and dialog together will lead to trouble later on.

ovv commented 6 years ago

There is a wait_for_ack parameter here:

https://github.com/Eyepea/aiosip/blob/master/aiosip/dialog.py#L156-L162

Would that work ?

vodik commented 6 years ago

I didn't testing it, but I don't see how it could work, since it registers itself inside that transaction table:

https://github.com/Eyepea/aiosip/blob/126c6404db80fc9fd8952056ef4a65bc393714c0/aiosip/dialog.py#L109-L112

So when the dialog is fed the ACK, it doesn't actually have a way to route it back to the particular transaction. I could be wrong, but I'd suspect wait_for_ack is broken and will just hang.

I'm not sure I understand what you mean ...

From RFC 3261:

12.1 Creation of a Dialog

   Dialogs are created through the generation of non-failure responses
   to requests with specific methods.  Within this specification, only
   2xx and 101-199 responses with a To tag, where the request was
   INVITE, will establish a dialog.  A dialog established by a non-final
   response to a request is in the "early" state and it is called an
   early dialog.  Extensions MAY define other means for creating
   dialogs.

The modelling we have is kinda wrong - we can't create a dialog out of thin air, its created with an INVITE, SUBSCRIBE, REGISTRATION, etc (this is what I meant by INVITE-like). In the case of an INVITE, if that first transaction fails, you don't have an established dialog to send other messages on.

There are also enough differences with INVITE-like that requires special considerations. The SIP timer rules are different. There's a state machine that needs to be implemented (see 17.1.1.2 in RFC 3261).

Then all that aside, we could simplify receiving other messages on a particular dialog by providing them through that async generator interface by default. This means:

And it probably would also simplify proxy implementation.

vodik commented 6 years ago

Okay, so I'm slightly off, as far as I can tell, provision responses only apply to invites.

Anyways, I'm putting together some basic support for this idea, implementing the full state machine as specified in RFC 3261, and adding a invite server/client example. The client currently looks like this:

    dialog = await peer.invite(
        from_details=from_details,
        to_details=to_details,
        payload=None,
    )

    async with dialog:
        # loops until we get a >200 state message (e.g. Completed or Terminated state)
        async for msg in dialog.wait_for_terminate():   # not sure if I like this
            print("CALL STATUS:", msg.status_code)
            # TODO: some sort of API to set the ACK payload to support late media offers

        print("CALL ESTABLISHED")
        # we now have a dialog in a valid state
        await dialog.request('OPTION', header={...})

        # instead of installing a router, or a default callback, I think it make sense to provider an async iterator interface for reading messages:
        async for msg in dialog:
            if msg.method == 'OPTION':
                do_something_useful()

    # __aexit__ will trigger with bye *or* cancel, depending on state
    print("CALL TERMINATED")

Another option, and this is where I'd like some feedback, is peer.invite could return some sort of box that unboxes a dialog once the call is established, but I don't think the two things can actually be separated - my understanding is that its the dialog would be invalid to use after a BYE is sent.

ovv commented 6 years ago

Thanks for the explanation it's a lot clearer now :). Your proposition looks really good, especially the first one.

I'm not sure dialog.wait_for_terminate() is really needed.

async with peer.invite() as dialog:
  async for msg in dialog:

Something like this coupled with a property like dialog.status is probably enough ?

The only downside I see is for future requests:

await dialog.request('OPTION', header={...})

Would the response come in the next async for msg in dialog or be the return value of dialog.request ?

Another option, and this is where I'd like some feedback, is peer.invite could return some sort of box that unboxes a dialog once the call is established, but I don't think the two things can actually be separated

As you probably have guess I'm not that familiar with the SIP RFC but it looks like added complexity for not much. If we add a dialog.status property we could make it clear that it's not yet a RFC compliant ?

vodik commented 6 years ago

So this is also why test_invite has a long delay as well. Because it times out waiting for the ack without actually throwing an error for some reason.