Azure / azure-sdk-for-python

This repository is for active development of the Azure SDK for Python. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/python/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-python.
MIT License
4.64k stars 2.84k forks source link

Error while reusing BatchMessage object #22529

Closed Vprashant closed 2 years ago

Vprashant commented 2 years ago

Describe the bug from this we have created service bus client : servicebus_client = ServiceBusClient.from_connection_string(conn_str=sender_connection_string)

created a sender object: sender = servicebus_client.get_topic_sender(topic_name=topic_name) batch_message = sender.create_message_batch()

So when we deepcopying the batch_message instance we got following issue topic_message_batch = copy.deepcopy(batch_message)

File "/usr/local/lib/python3.8/site-packages/uamqp/message.py", line 141, in getstate state = deepcopy(state, memo) File "/usr/local/lib/python3.8/copy.py", line 146, in deepcopy state["state"] = self.state.value AttributeError: 'BatchMessage' object has no attribute 'state'

while using 7.0.0 of azure_servicebus we haven't faced any issues and everying was running fine.

mccoyp commented 2 years ago

Hi @Vprashant, thank you for opening an issue! I'll tag the appropriate folks so we can look into this as soon as possible.

swathipil commented 2 years ago

Hey @Vprashant - apologies for the late response! I was able to reproduce the error with azure-servicebus==7.4.0, and I believe that this is related to some changes that were made in the underlying uamqp library dependency. More specifically, when we added support for pickling to uamqp==1.3.0: https://github.com/Azure/azure-uamqp-python/blob/master/HISTORY.rst#130-2021-04-05.

You may not have noticed these changes (starting 7.1.1) if you updated directly from azure-servicebus==7.0.0 (uamqp < 1.3.0) to azure-servicebus==7.4.0 (uamqp >= 1.4.3).

Just to double check, can you confirm the uamqp dependency versions that you are using for servicebus 7.0.0 and 7.4.0?

Vprashant commented 2 years ago

Hey @swathipil - uamqp dependency versions

uamqp==1.2.12 azure-servicebus==7.0.0

uamqp==1.4.3 azure-servicebus==7.4.0 Yes , I haven't used 7.1.1 , I directly move to azure-servicebus==7.4.0 from azure-servicebus==7.0.0

swathipil commented 2 years ago

@Vprashant thanks for the quick response! As a follow-up: You mentioned that copy.deepcopy(message_batch) worked when you used azure-servicebus 7.0.0 and uamqp 1.2.12. When I try to do the same, I get the following error: File "stringsource", line 2, in uamqp.c_uamqp.StringValue.__reduce_cython__ TypeError: no default __reduce__ due to non-trivial __cinit__. Is this not an error that you encounter? (It's surprising that you didn't, given that we originally added the uamqp pickling support to resolve this error when pickling.)

Vprashant commented 2 years ago

Hi @swathipil, No, I had't faced that above mentioned error; while working with azure-servicebus 7.0.0 and uamqp 1.2.12

MicrosoftTeams-image

swathipil commented 2 years ago

Hey @Vprashant - thanks for your patience!

I tested azure-servicebus==7.0.0 with the uamqp version you had, and I am still getting the non-trivial __cinit__ error. Would you be able to run pip freeze for this scenario that was working and share it here?

The error that you are seeing with azure-servicebus==7.4.0 is from the underlying uamqp.BatchMessage C object. We would need to overload the __reduce__ method in BatchMessage. To modify the uamqp library and release it, it'll take a little bit of time.

As a temporary solution, would you be able to add the ServiceBusMessage objects to a list, copy the list, and send the messages? The ServiceBusMessage objects can be deepcopied. Example here:

sender = servicebus_client.get_topic_sender(topic_name=topic_name)
messages = [ServiceBusMessage("Message in list") for _ in range(10)]
messages_copy = copy.deepcopy(messages)
sender.send_messages(messages)
sender.send_messages(messages_copy)

Just in case you are curious, here are a few issues related to this same error: https://github.com/Azure/azure-sdk-for-python/issues/12901 https://github.com/Azure/azure-sdk-for-python/issues/15772

Vprashant commented 2 years ago

Hi @swathipil ,We are using a premium tier service and we need to use batching in our solution, for azure-servicebus==7.0.0 version. In my last message I mention the version in the attached image ( pip list | grep azure-servicebus).

$ pip freeze | grep "azure" azure-common==1.1.26 azure-core==1.9.0 azure-eventhub==5.2.0 azure-functions==1.7.2 azure-servicebus==7.0.0 $ pip freeze | grep "uamqp" uamqp==1.2.12

So for now, the temporary solution is not solving our batching issue. Let me know if you find something that works on batching service.

swathipil commented 2 years ago

Will update you on our plans after I discuss with the team. Thanks!

swathipil commented 2 years ago

Hey @Vprashant - we were able to fix this issue in the following PR: https://github.com/Azure/azure-uamqp-python/pull/310

This will fix will be out in the next uamqp release.

This fix will also be out in the next azure-servicebus release. To avoid waiting for the azure-servicebus release, you can install uamqp==1.5.2 as soon as it has been released.

More details about the problem/solution if you're interested: The uamqp.BatchMessage inherited __getstate__/__setstate__ methods (used in copy.deepcopy) from the Message superclass. The methods needed to be overridden in BatchMessage.

ghost commented 2 years ago

Hi @Vprashant. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text “/unresolve” to remove the “issue-addressed” label and continue the conversation.

ghost commented 2 years ago

Hi @Vprashant, since you haven’t asked that we “/unresolve” the issue, we’ll close this out. If you believe further discussion is needed, please add a comment “/unresolve” to reopen the issue.

swathipil commented 2 years ago

@Vprashant - uamqp==1.5.2 has been released. We will not be bumping the minimum uamqp dependency version in azure-servicebus, since this issue did not require any updates to servicebus. However, as long as you have uamqp==1.5.2 installed, you should be able to deepcopy the ServiceBusMessageBatch. Thanks!