Fix: should attach if the channel is in the FAILED state

[ricardopereira]

#485

(RTL4g) If the channel is in the FAILED state, the attach request sets its errorReason to null, and proceeds with a channel attach described in RTL4b, RTL4h and RTL4c

Please only merge if all tests are passing.
This can break other tests like RTP11b.

[ricardopereira]

Oh my, this has been already fixed by @EvgenyKarkan with #497.
I'm still 💤💤💤.
Sorry @EvgenyKarkan, did you see this? I'll change RTC8 back to not pending when one of the fixes is merged and again only merge if all tests are passing because it could break other things.

[EvgenyKarkan]

did you see this?

@ricardopereira sorry, it seems I missed this thread (

this has been already fixed

yes, I did a try to fix #485 , but Toni detected regression which I did not investigate yet (is it legacy or not).

[ricardopereira]

The change I did for RTL4g broke the RTP11b test.

(RTP11b) Implicitly attaches the Channel if the channel is in the INITIALIZED state. However, if the channel is in or enters the DETACHED or FAILED state before the operation succeeds, it will result in an error

I think RTP11b should be updated to

"Implicitly attaches the Channel if the channel is in the INITIALIZED state. However, if the channel is in or enters the DETACHED or FAILED state before the operation succeeds, it will result in an error"

because

• if the channel is DETACHED then attach
• if the channel is FAILED then clear the error and attach (RTL4g)

[ricardopereira]

@mattheworiordan PTAL

[tcard]

@ricardopereira I'm not against your proposal per se, but why can't we just stick to what RTP11b says, as is? If the change you did for RTL4g broke the RTP11b test, shouldn't we fix our RTP11b implementation rather than changing the RTP11b spec?

[ricardopereira]

@tcard I did fix RTP11b. It's working fine now but when I revisited the RTP11b spec I noticed that some parts were inconsistent. Like I said before, the RTP11b states that if the channel is in DETACHED or FAILED state then the operation should result in an error and that's not what should be happening.

[mattheworiordan]

@tcard I did fix RTP11b. It's working fine now but when I revisited the RTP11b spec I noticed that some parts were inconsistent. Like I said before, the RTP11b states that if the channel is in DETACHED or FAILED state then the operation should result in an error and that's not what should be happening.

There is a good reason for this behaviour. If the channel, in the 0.9 spec, is in the FAILED state it is terminal. If it is in the DETACHED state, then the user has explicitly set it to DETACHED (the server never moves it to DETACHED, only to SUSPENDED now).

So if a channel is in a terminal state, we should not implicitly try and fix that. They should be trying to fix that explicitly.

If the channel has been explicitly detached by the user, and they call get, then we should not implicitly attach again as the implicit attach is ONLY a convenience for initialized channels to save the user having to explicitly attach before doing an operation. That implicit attach never happens again because the convenience opportunity has passed.

So I believe the spec is indeed right as it stands as we don't want implicit actions unless the channel is initialized.

[ricardopereira]

@mattheworiordan Thanks, understood. I will write the missing parts of the spec.

[ricardopereira]

@mattheworiordan PTAL to the changes I made.

[tcard]

LGTM

(BTW, I thought I would die before seeing the CI build passing again! Feels nice.)

[tcard]

Minor nitpick: you can do

case ARTRealtimeChannelFailed:
case ARTRealtimeChannelDetached:
    if (callback) callback(nil, [ARTErrorInfo createWithCode:0 message:msg]);
    return;

and save yourself two lines :)

[ricardopereira]

🤦 Damn, you're right. Forgot about the fallthrough.

[ricardopereira]

Changed 👍

[ricardopereira]

Autosquashed.