[PyAMQP] Connections TODO #25802

kashifkhan · 2022-08-22T14:37:04Z

This PR tries to address 2 TODOs for Connection

_incoming_open : if a connection flow during Open receives an illegal state, how do we handle. Today uamqp & pyamqp both ignore it. Right now I've set it that if any other state appears while opening, close. Looking at some other libraries and the amqp specs, there doesnt seem to be any defined thing that should happen if the Open process goes off path, so it might be open to the implementer ?
_incoming_close: How to handle a close frame on an invalid channel. I moved the todos around popping the channels on both incoming and outgoing since the spec mentions either party can close and then they mutually close. If the channel specified is not found, then is it an log and move on or close the entire thing with an error. Ive chosen the latter now

azure-sdk · 2022-08-22T15:02:28Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-eventhub

kashifkhan · 2022-08-22T15:15:39Z

/azp run python - eventhub - tests

azure-pipelines · 2022-08-22T15:15:57Z

Azure Pipelines successfully started running 1 pipeline(s).

l0lawrence · 2022-08-22T15:27:50Z

sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/_connection.py

@@ -507,10 +510,13 @@ def _incoming_end(self, channel, frame):
        """
        try:
            self._incoming_endpoints[channel]._incoming_end(frame)  # pylint:disable=protected-access
+            self._incoming_endpoints.pop(channel)


Just curious: For an outgoing_close, could we receive a response from the service on that channel to confirm it is closing? (i.e from the incoming_endpoint channel). Edit: Or is that not relevant since they mutually close?

I think if the entire Connection was being closed, then the state would be set to END after sending after sending a CLOSE to the other side. In this case while this channel has closed, there might be other channels still open. Unless I missed it here, doesnt seem to be the case in channels http://docs.oasis-open.org/amqp/core/v1.0/csprd01/amqp-core-transport-v1.0-csprd01.html#doc-idp191152

It's actually a really good question - and why this was a TODO in the first place.
It's hard to know where to pop the channel/session/link because if it's initiated by us, we don't always wait around for the service response.
So we need to test/be able to handle:

If we get an incoming close/end/detach, then is this in response to one we already sent? If not, we can probably just send the response frame then pop it.

If we are sending a close/end/detach, if we pop it then we need to be able to handle additional incoming frames that are sent to it. Otherwise, do we wait until we explicitly receive the response? That can be slow and messy if there's other things going on in the connection.

kashifkhan · 2022-08-22T19:26:18Z

/azp run python - eventhub - tests

azure-pipelines · 2022-08-22T19:26:31Z

Azure Pipelines successfully started running 1 pipeline(s).

annatisch · 2022-08-23T03:52:59Z

sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/_connection.py

-            pass  # TODO: channel error
-        #self._incoming_endpoints.pop(channel)  # TODO
-        #self._outgoing_endpoints.pop(channel)  # TODO
+            end_error = AMQPError(condition=ErrorCondition.InvalidField, description=f"Invalid channel {channel}", info=None)


Is InvalidField the best error condition here?

Changed it to ClientError since if a channel is missing from the dictionary (either when the frame comes from the client itself or the service) its something the SDK didn't handle on close properly.

sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/_connection.py

annatisch · 2022-09-01T20:10:20Z

sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/_connection.py

-        #self._outgoing_endpoints.pop(channel)  # TODO
+            end_error = AMQPError(condition=ErrorCondition.ClientError, description=f"Invalid channel {channel}", info=None)
+            _LOGGER.error(f"Invalid channel {channel} ")
+            self.close(error=end_error)


ErrorCondition.ClientError is not a protocol-supported condition - so only use if for errors generated for the sole purpose of raising exceptions to users.
If we are actually building an AMQPError object to send in a CLOSE frame - it should use one of the protocol conditions.

annatisch · 2022-09-01T20:10:48Z

sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/aio/_connection_async.py

@@ -305,7 +305,11 @@ async def _incoming_open(self, channel, frame):
            await self._outgoing_open()
            await self._set_state(ConnectionState.OPENED)
        else:
-            pass  # TODO what now...?
+            # TODO what now...?


We could remove the todo?

check-enforcer · 2022-09-02T17:50:33Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run python - [service] - ci

kashifkhan added 3 commits August 22, 2022 08:51

illegal Connection state

ac059de

handling illegal channel

c528540

fix

0c55b95

kashifkhan requested review from annatisch, swathipil and l0lawrence as code owners August 22, 2022 14:37

ghost added the Event Hubs label Aug 22, 2022

l0lawrence reviewed Aug 22, 2022

View reviewed changes

async changes

f530c0c

annatisch reviewed Aug 23, 2022

View reviewed changes

sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/_connection.py Outdated Show resolved Hide resolved

kashifkhan added 2 commits August 23, 2022 09:37

fix logging

24c25af

change invalid code

2ad46d6

annatisch reviewed Sep 1, 2022

View reviewed changes

remove TODO

73fd169

kashifkhan added 2 commits September 2, 2022 13:39

protocol error for invalid channel on receive

3278207

resolve merge errors

1fafcfb

kashifkhan requested review from scbedd, benbp, weshaggard, BigCat20196, msyyc, lmazuel and chlowell as code owners September 2, 2022 18:54

kashifkhan requested review from a team, gkostal, xiangyan99, YalinLi0312, jsquire and ronniegeraghty as code owners September 2, 2022 18:54

kashifkhan removed request for a team, gingi, dpwatrous, jsquire, benbp, lmazuel, gkostal, weshaggard, chlowell, zfengms, rakshith91, xiangyan99, cRui861, YalinLi0312, ronniegeraghty, kristapratico, paterasMSFT, anilba06, scbedd, BigCat20196 and msyyc September 2, 2022 18:55

kashifkhan closed this Sep 2, 2022

kashifkhan mentioned this pull request Sep 2, 2022

[PyAMQP] Connections TODO #26018

Merged

kashifkhan deleted the pyamqp_todos branch September 7, 2022 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PyAMQP] Connections TODO #25802

[PyAMQP] Connections TODO #25802

kashifkhan commented Aug 22, 2022

azure-sdk commented Aug 22, 2022

kashifkhan commented Aug 22, 2022

azure-pipelines bot commented Aug 22, 2022

l0lawrence Aug 22, 2022 •

edited

Loading

kashifkhan Aug 22, 2022

annatisch Aug 23, 2022

kashifkhan commented Aug 22, 2022

azure-pipelines bot commented Aug 22, 2022

annatisch Aug 23, 2022

kashifkhan Aug 24, 2022

annatisch Sep 1, 2022

annatisch Sep 1, 2022

check-enforcer bot commented Sep 2, 2022

[PyAMQP] Connections TODO #25802

[PyAMQP] Connections TODO #25802

Conversation

kashifkhan commented Aug 22, 2022

azure-sdk commented Aug 22, 2022

kashifkhan commented Aug 22, 2022

azure-pipelines bot commented Aug 22, 2022

l0lawrence Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

kashifkhan Aug 22, 2022

Choose a reason for hiding this comment

annatisch Aug 23, 2022

Choose a reason for hiding this comment

kashifkhan commented Aug 22, 2022

azure-pipelines bot commented Aug 22, 2022

annatisch Aug 23, 2022

Choose a reason for hiding this comment

kashifkhan Aug 24, 2022

Choose a reason for hiding this comment

annatisch Sep 1, 2022

Choose a reason for hiding this comment

annatisch Sep 1, 2022

Choose a reason for hiding this comment

check-enforcer bot commented Sep 2, 2022

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

l0lawrence Aug 22, 2022 •

edited

Loading