search

Azure / azure-iot-sdk-node

A Node.js SDK for connecting devices to Microsoft Azure IoT services
https://docs.microsoft.com/en-us/azure/iot-hub/
Other
261 stars 227 forks source link

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

Closed YoDaMa closed 4 years ago

YoDaMa commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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.