Problem/Motivation
Currently if a DB supporting transactions is used then on entity save the transaction will be committed after the SqlContentEntityStorage::save() method is finished and the transaction object is out of scope so that its destructor is executed. This means that all the hooks will be fired before the transaction is committed including the update hook.
If you are building a system where a lock for the entity is being created in the presave hook and should be released after the entity is saved then using the update hook while in transaction might lead to race conditions e.g. by releasing the lock and a concurrent request trying to get a lock and load the entity with the expectation that the entity will be the newly saved entity by the other process. However this will not be the case if it happens that the lock is retrieved by the concurrent process just before the transaction is committed in which case the concurrent process will load the entity in its previous state.
In order to overcome this we need a hook that will be fired after the transaction is committed.
Proposed resolution
In SqlContentEntityStorage::save() commit the transaction and start a new one inside which invoke an entity post transactional hook.
Remaining tasks
Decide between #25 and #33 for proposed solution
Add tests
Review
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#33 | 2892654-33.patch | 1.09 KB | kunal_sahu |
#25 | 2892654-25.patch | 1.91 KB | Megha_kundar |
#24 | 2892654-24.patch | 1.96 KB | Megha_kundar |
#23 | 2892654-93.patch | 1.95 KB | Megha_kundar |
#20 | 2892654-93.diff | 1.95 KB | jitendrapurohit |
Issue fork drupal-2892654
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
hchonovComment #5
pclaitte CreditAttribution: pclaitte as a volunteer and commentedHello
Thanks for the patch. You save my day.
I was into a hook_file_update and I wanted to access file_usage but she was was not yet updated because transaction had not been commited. I have moved my code into the new hook_ENTITY_TYPE_post_transaction and it works perfectly.
Thanks a lot
Comment #6
Wim LeersAlso related: #2966607: Invalidating 'node_list' and other broad cache tags early in a transaction severely increases lock wait time and probability of deadlock.
Comment #8
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI just now opened #2992426: Add entity methods and hooks for executing pre/post code outside of the save transaction to propose doing a bit more than what this issue is currently scoped to.
Comment #9
joseph.olstadI need this for doing metatag overrides , easier if the custom stuff I want to do is on post save.
Comment #10
joseph.olstadpatch still applies, but with fuzz, could use a reroll.
I'm going to use it.
Comment #11
joseph.olstadI applied this core patch, after applying it I was unable to do a cache rebuild with drush.
So I reverted it.
we are using drush 9.2.3
Comment #12
hchonovI don't think that your problems are because of the patch. I don't see how it could be affecting anything, as it only adds functionality in the entity's saving process. You've mentioned that you need a re-roll. Do you mind uploading it?
Also please share the complete error message / backtrace.
Comment #13
joseph.olstadI made no modifications to the patch.
sorry I don't have time right now to provide more in dept analysis.
reverted it for now. Might try it again later however I discovered that I did not need the patch.
For now we're using hook_presave instead and it does the job for us.
Comment #16
Wim Leers#2966607: Invalidating 'node_list' and other broad cache tags early in a transaction severely increases lock wait time and probability of deadlock landed, which probably makes this more feasible.
Comment #20
jitendrapurohit CreditAttribution: jitendrapurohit commentedThe patch doesn't apply on the latest drupal 9.3. Here's the updated patch.
Comment #23
Megha_kundar CreditAttribution: Megha_kundar at Srijan | A Material+ Company for Drupal Association commentedUpdated patch file extension from .diff to .patch
Comment #24
Megha_kundar CreditAttribution: Megha_kundar at Srijan | A Material+ Company for Drupal Association commentedComment #25
Megha_kundar CreditAttribution: Megha_kundar at Srijan | A Material+ Company for Drupal Association commentedComment #26
Megha_kundar CreditAttribution: Megha_kundar at Srijan | A Material+ Company for Drupal Association commentedComment #27
masterperoo CreditAttribution: masterperoo commentedConfirmed - this saved my day as well!
Im my case - there are field values that need updating after the entity has been saved/inserted and is already visible to other modules.
Without this hook - I am facing splitting every entity save into separate chunks.
the patch is a life saver!
Comment #29
joachim CreditAttribution: joachim at Factorial GmbH commentedThis needs more detail.
Also, it should say that this hook is only available to entities using SQL storage.
Comment #30
joachim CreditAttribution: joachim at Factorial GmbH commentedOops. wrong status
Comment #31
Mykola DolynskyiCore firing hook_entity_insert() and hook_entity_insert() before transaction is finished may be not so bad, but bad there are no same hooks to be fired after transaction finished and no tables locked!
Comment #33
kunal_sahu CreditAttribution: kunal_sahu at Valuebound for Valuebound commentedThe above patch is committing the transaction and then starting a new one inside the if block that checks if the parent save() method has returned true. This ensures that the post transactional hook is only invoked if the entity save operation was successful.
The moduleHandler is used to invoke the entity_post_transactional_save hook, which should be implemented by any modules that need to perform actions that are not part of the main entity save transaction.
Please review. Thanks
Comment #34
smustgrave CreditAttribution: smustgrave at Mobomo commented@kunal_sahu can you explain why your solution is a better approach then the previous patches please.
Added to the remaining tasks decision needs to be made between #23 and #33.
Test coverage will be needed either wya.