feat(azure-iot-device): adds emit to connect

[YoDaMa]

No description provided.

[anthonyvercolano]

Weren't we told not to send async to functions that weren't expecting them? You don't send async's as any of the other callbacks.

[anthonyvercolano]

One is past tense the other isn't.

[anthonyvercolano]

🕐

[BertKleewein]

this._transport.on('connected', () => this.emit('connect')); [](start = 4, length = 60)

There's an asymmetry here between when we emit('connect') and when we emit('disconnect').

'connect' is emitted whenever the transport FSM enters the "connected" (mqtt) or "authenticated" (amqp) state. No matter what. I think that's good.

'disconnect', however, has some big caveats.

In the transport:

1 For MQTT, it is not passed up (emitted) to the client if the disconnect is intentional (disconnectCallback is set becaues mqtt.disconnect was called with a callback or mqtt.connect was called with a callback but failed). If mqtt.disconnect was called without a callback or if mqtt.connect was called without a callback and failed, then it is passed up to the client no matter what.

2. For AMQP, it is passed up if there is an error updating the sas token or if common/amqp calls the setDisconnectHandler callback. If there's an error updating the sas token and the transport is disconnected as a result, 2 events might be passed up. I didn't dig into common/amqp to see when the setDisconnectHandler callback gets called.

Then, in the client:

3. It isn't emitted if the disconnect was caused by a retryable error, unless the retry fails to re-enable twins or methods. If both twin and methods fail to be re-enabled, then it gets emitted twice. If neither twins or methods are enabled, then it is never emitted.

4. If the disconnect wasn't caused by an error, or if the error isn't retryable, then it does get emitted.

I feel your pain, brother, but I'm not sure how reliable this is going to be.

[anthonyvercolano]

@BertKleewein AMQP support wasn't really factored into this PR too much. It should come along later if needed.

In inclined to agree that we need to look more at the device and common transports. My own wonder is how do we deal with MQTTs inherent disconnect/connect on re-authorization. Does the client application really want to know about that?

I'm imagining that with some of your testing we'll quickly iterate to the right emission of both the disconnect and the connect. (In deference to your analysis, I was more worried about the connect emits as this is the newer action)

[YoDaMa]

OK, so @BertKleewein, I appreciate your analysis of the disconnect logic. It is.... not simple? Should it be simple? Is it right to be as complex as it currently is...? Let me respond in sections:

Stuff for this PR

1. As it relates to this PR, I do not want to touch disconnect. I think that is another PR if we are to reconsider how the event invocation operates. As such for this PR I'm inclined to box up your comments and move them into a conversation about reworking disconnect in another PR. I hope you are ok with this logic. I'm disinclined toward creating large, expansive PRs that have multiple goals :) simple is better.

2. connect event.

@anthonyvercolano you're saying that we should look more at the common transports for dealing with these event emissions? I mean... We could... But what's the definition of when connect should be emitted to the user?

I am a simpleton, so my brain first said this: anytime we enter a conneted state. The reason is because I don't want to have users confused if their application is supposed to be receiving a connect even but we've been too clever and said well we don't think in this specific instance you want to receive a connect event. We would also have to make sure if we do start getting clever that we document the h*ll out of how it works. We can do that! But historically documentation falls apart... intuitive APIs seem like the best choice.

Finally, @anthonyvercolano, ur comment:

In inclined to agree that we need to look more at the device and common transports. My own wonder is how do we deal with MQTTs inherent disconnect/connect on re-authorization. Does the client application really want to know about that?

this is a good point... from the code, it looks that the logic for AMQP and MQTT reauth transitions happen at the common layer, i.e. there are alot of state changes going on with SAS token updates, but on the device layer... there are no such transitions! This is good in my opinion... Maybe a caveat for the more thoughtful users to include in documentation, that if there is a SAS related disconnect/connect cycle, then the connect event will not be emitted.

Does this sound good?

Also @anthonyvercolano what changes do you still have in mind for requested changes? I'm not clear on that any more since I changed the past tense to the present tense (for your grammatics)

[BertKleewein]

I agree that we can leave disconnect alone for now. My big concern is that I was worried about losing events. e.g. the transport gets disconnected but the event never fires. My secondary concern is about getting double events. e.g. you're already connected and you get another connected event.

Disconnect/reconnect because of SAS renewal? My intuition says to shield the user from this, but I'm not sure.

The timing of "connected" gets weird when you add subscribing and sasl. Are you "connected" once you get a CONNACK, or do you have to wait until after you're done subscribing (If this happens as part of the connect). Do you need to wait until the sasl auth is complete and all the channels are open? I don't know the answer to this. I'm just pointing out that there can be more than one definition of "connected"

Saying "every time you enter the connected state" is simple for you, but you're leaking the state machine logic into the API. If you're in "connected", and then you go into "subscribing" and back to "connected", then you're entering the connected state again. The only reason you would fire the event in this case is because that's what the state machine is doing.

The problem is that "connected" from the user's standpoint is different than "connected" from the state machine standpoint. There are many different state machine states that could be considered "connected" and there are many different state machine transitions that could mean "I was previously disconnected but now I am connected". Likewise with disconnected and the connected->disconnected transitions.

I could go on a whole rant here on how state machines are broken, but that's a different conversation that I'd be glad to go into (just not here and not now :))

[YoDaMa]

@BertKleewein

Disconnect/reconnect because of SAS renewal? My intuition says to shield the user from this, but I'm not sure.

My intuition says don't show this to users. So in MQTT and AMQP this PR would shield this from users, because the device level state machine does not transition states when updating the SAS Token.

I agree, we do not necessarily want to 1-1 expose the internal state machine to the user... HOWEVER, the device state machine for connected seems to match the intuitive way a connect event would function.

Do you need to wait until the sasl auth is complete and all the channels are open? I don't know the answer to this. I'm just pointing out that there can be more than one definition of "connected".

I am honestly not well acquainted enough with sasl auth to know the answer to this. I also don't know where sasl auth is used in the device client library.

[anthonyvercolano]

What's this doing here?

[anthonyvercolano]

Enter in a backlog item now to address the issues that @bert Kleewein raised with disconnects with twin and methods. We want to make sure that this is clean. See if we can test correct action at the unit testing phase.

[anthonyvercolano]