Close AMQP connection explicitly when no more links #4914
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ept tcp level connection)
| @@ -21,18 +22,21 @@ namespace Microsoft.Azure.Devices.Edge.Hub.Amqp | |||
| /// </summary> | |||
| class ClientConnectionHandler : IConnectionHandler | |||
| { | |||
| readonly TimeSpan closeTimeout = TimeSpan.FromSeconds(10); | |||
There was a problem hiding this comment.
I know still draft, sorry for jumping the gun... but how this number was picked? maybe consider using "CloudOperationTimeout" setting here,
There was a problem hiding this comment.
I've checked the option to inject the timeout as a parameter, but it would go through several objects from the point of dependency registration and most of the time I had the feeling as "it does not belong there". Also, CloudOperationTimeout regards to cloud operations while this one is about talking to devices, so then the best would be to create its own (or a more general) timeout value.
Instead what I did, I checked in the amqp source code what it does and I saw that in the newer version there is a CloseAsync() with no timeout, which uses a hardcoded 60 sec timeout value. So I brought that value here and using that. When the time passes, the code throws a timeout exception
…r amqp api for CloseAsync() without timeout
vadim-kovalyov
approved these changes
May 10, 2021
vipeller
added a commit
to vipeller/iotedge
that referenced
this pull request
May 14, 2021
darobs
pushed a commit
to darobs/iotedge
that referenced
this pull request
May 26, 2021
…ept tcp level connection) (Azure#4984) cherry pick of Azure#4914
ggjjj
pushed a commit
to ggjjj/iotedge
that referenced
this pull request
Jul 22, 2021
This fixes the following problem: In certain situations EdgeHub can decide to close the connection of a device (or module). Such situations are when a SAS token expires or the device gets disabled on the azure portal, for example. The current solution to close the device when it uses Amqp is to close the links in the Amqp session. The problem is what the underlying transport channel remains open in this case and the device is able to reopen the channel. When EdgeHub closes a client that way it also sets its representing device objects (the related instance of classes like DeviceProxy) as closed. After that moment for certain operations, like sending M2M messages, EdgeHub checks if a module has an active DeviceProxy, and if does not, it does not send out the message. Interestingly, other operations (like transmitting twin results) does not do this check and the operation goes through, as the underlying link can be reopened. Also, when the module (or device) does an operation (e.g. sending a telemetry message) that can go through, even if the device proxy is closed. Because of this bug, the following can happen: 1) The module works properly for a while 2) The token is near to expire, so the module sends a new one. Let's say that the processing of the token has problems (in real life scenario, talking to edged failed for some reason), so EdgeHub chooses to close the connection. 3) EdgeHub closes all the Amqp links, and sets the device proxy as closed/ 4) The module wants to send a telemetry message, it still have the underlying transport, so it opens a new link for telemetry, and sends the message. 5) EdgeHub processes the message and everything seems working 6) An incoming message comes from somewhere which is routed to the module we are talking about. 7) EdgeHub finds the module and sees that the device proxy is closed, so it does not send the message. The message does not get lost, but will not be sent out until the module does not get restarted. Also if there is a time-to-leave value (or some other limits), the message will be dropped after a while. This fix changes the logic from closing the links to close the entire connection. As a result the client (device or module) cannot reopen links but needs to create a new connection, which results creating a new DeviceProxy with an appropriate state (=opened), so M2M messages (and other device bound operations checking the state) can go through.
damonbarry
pushed a commit
to damonbarry/iotedge
that referenced
this pull request
Apr 15, 2022
This fixes the following problem: In certain situations EdgeHub can decide to close the connection of a device (or module). Such situations are when a SAS token expires or the device gets disabled on the azure portal, for example. The current solution to close the device when it uses Amqp is to close the links in the Amqp session. The problem is what the underlying transport channel remains open in this case and the device is able to reopen the channel. When EdgeHub closes a client that way it also sets its representing device objects (the related instance of classes like DeviceProxy) as closed. After that moment for certain operations, like sending M2M messages, EdgeHub checks if a module has an active DeviceProxy, and if does not, it does not send out the message. Interestingly, other operations (like transmitting twin results) does not do this check and the operation goes through, as the underlying link can be reopened. Also, when the module (or device) does an operation (e.g. sending a telemetry message) that can go through, even if the device proxy is closed. Because of this bug, the following can happen: 1) The module works properly for a while 2) The token is near to expire, so the module sends a new one. Let's say that the processing of the token has problems (in real life scenario, talking to edged failed for some reason), so EdgeHub chooses to close the connection. 3) EdgeHub closes all the Amqp links, and sets the device proxy as closed/ 4) The module wants to send a telemetry message, it still have the underlying transport, so it opens a new link for telemetry, and sends the message. 5) EdgeHub processes the message and everything seems working 6) An incoming message comes from somewhere which is routed to the module we are talking about. 7) EdgeHub finds the module and sees that the device proxy is closed, so it does not send the message. The message does not get lost, but will not be sent out until the module does not get restarted. Also if there is a time-to-leave value (or some other limits), the message will be dropped after a while. This fix changes the logic from closing the links to close the entire connection. As a result the client (device or module) cannot reopen links but needs to create a new connection, which results creating a new DeviceProxy with an appropriate state (=opened), so M2M messages (and other device bound operations checking the state) can go through.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the following problem:
In certain situations EdgeHub can decide to close the connection of a device (or module). Such situations are when a SAS token expires or the device gets disabled on the azure portal, for example.
The current solution to close the device when it uses Amqp is to close the links in the Amqp session. The problem is what the underlying transport channel remains open in this case and the device is able to reopen the channel.
When EdgeHub closes a client that way it also sets its representing device objects (the related instance of classes like DeviceProxy) as closed. After that moment for certain operations, like sending M2M messages, EdgeHub checks if a module has an active DeviceProxy, and if does not, it does not send out the message. Interestingly, other operations (like transmitting twin results) does not do this check and the operation goes through, as the underlying link can be reopened.
Also, when the module (or device) does an operation (e.g. sending a telemetry message) that can go through, even if the device proxy is closed.
Because of this bug, the following can happen:
The message does not get lost, but will not be sent out until the module does not get restarted. Also if there is a time-to-leave value (or some other limits), the message will be dropped after a while.
This fix changes the logic from closing the links to close the entire connection. As a result the client (device or module) cannot reopen links but needs to create a new connection, which results creating a new DeviceProxy with an appropriate state (=opened), so M2M messages (and other device bound operations checking the state) can go through.