magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.55k stars 9.32k forks source link

Magento\Framework\MessageQueue\PublisherInterface::publish() should never (ever) be called from code which is inside of active DB transaction #39053

Closed pczerkas closed 2 weeks ago

pczerkas commented 2 months ago

Preconditions and environment

Steps to reproduce

If queue message is published inside of open DB transaction, and message consumer processes it faster then DB transaction is commited, then message consumer (which is separate process from publisher) has not consistent view on data in DB, which leads to bugs.

Expected result

Queue messages can only be published, if calling code is NOT inside of active DB transaction.

Actual result

Queue messages ARE published, when calling code IS inside of active DB transaction.

Example: https://github.com/magento/magento2/blob/6f4805f82bb7511f72935daa493d48ebda3d9039/app/code/Magento/AsynchronousOperations/Model/BulkManagement.php#L131

            $this->entityManager->save($bulkSummary);

            $this->publishOperations($operations);

            $connection->commit();

Additional information

Problem can be resolved (in ideal scenario) by adding before plugin on interface Magento\Framework\MessageQueue\PublisherInterface::publish(), which checks if getTransactionLevel() on any active DB connection is not greater than 0.

This would probably be backward-incompatible change.

Release note

No response

Triage and priority

m2-assistant[bot] commented 2 months ago

Hi @pczerkas. Thank you for your report. To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, Add a comment to the issue:

m2-assistant[bot] commented 2 months ago

Hi @engcom-November. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

engcom-November commented 2 months ago

Hello @pczerkas,

Thank you for the report and collaboration!

Can you let us know in which module or while calling which consumer this issue has occured.

pczerkas commented 2 months ago

Can you let us know in which module or while calling which consumer this issue has occured.

Hi @engcom-November , this issue is about general design here.

We've seen that problem happening in consumer async.operations.all - sometimes first operation in bulk is not marked as done in DB.

pczerkas commented 2 months ago

@engcom-November , please look at https://github.com/magento/magento2/pull/39057 where is proposed fix. Looks like tests are passing OK.

m2-assistant[bot] commented 2 months ago

Hi @engcom-Hotel. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

engcom-Hotel commented 4 weeks ago

Hello @pczerkas,

Thank you for the report and your collaboration!

Your description seems valid to us. However, could you please provide details of the real-world scenario in which the flow is failing? We will try to replicate the scenario on our end to see if the issue is reproducible.

Thanks

engcom-Hotel commented 2 weeks ago

Hello @pczerkas,

This issue is being closed since it has not been updated in a long time. Please feel free to reopen or raise a new ticket if the issue still exists.

Thanks