[Messenger] Add clear Entity Manager middleware #31334
Merged
Conversation
This file contains hidden or 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
linaori
reviewed
May 1, 2019
src/Symfony/Bridge/Doctrine/Messenger/DoctrineClearEntityManagerMiddleware.php
Show resolved
Hide resolved
Tobion
reviewed
May 7, 2019
src/Symfony/Bridge/Doctrine/Messenger/DoctrineClearEntityManagerMiddleware.php
Outdated
Show resolved
Hide resolved
4c30a13 to
8c03f5f
Compare
sroze
reviewed
May 12, 2019
src/Symfony/Bridge/Doctrine/Messenger/DoctrineClearEntityManagerMiddleware.php
Outdated
Show resolved
Hide resolved
8c03f5f to
42ac426
Compare
1a25291 to
5d2b022
Compare
|
I've switched branch to 4.4, tests are green. |
sroze
approved these changes
Jun 2, 2019
Tobion
suggested changes
Jun 2, 2019
src/Symfony/Bridge/Doctrine/Tests/Messenger/DoctrineClearEntityManagerMiddlewareTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bridge/Doctrine/Tests/Messenger/DoctrineClearEntityManagerMiddlewareTest.php
Outdated
Show resolved
Hide resolved
5d2b022 to
6e690a6
Compare
|
Status: Needs Review |
Tobion
approved these changes
Jun 3, 2019
fabpot
approved these changes
Jun 4, 2019
|
Thank you @Koc. |
fabpot
added a commit
that referenced
this pull request
Jun 4, 2019
This PR was merged into the 4.4 branch. Discussion ---------- [Messenger] Add clear Entity Manager middleware | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #29662 | License | MIT | Doc PR | TBD General purpose of this middleware: * avoid memory leaks during messages handling * prevent unexpected side effects when entities that already stored in identity map not refreshed between messages Commits ------- 6e690a6 Add clear Entity Manager middleware (closes #29662)
vudaltsov
reviewed
Aug 5, 2019
| * | ||
| * @author Konstantin Myakshin <molodchick@gmail.com> | ||
| */ | ||
| class DoctrineClearEntityManagerMiddleware implements MiddlewareInterface |
|
I discovered this PR through Symfony blog post, and tried code of this PR on a Symfony 4.3 project. This middleware is correctly called by // get whatever user in $user variable
$message = new SendEmailCommand();
// Perform whatever business logic and add a stamp
$e = new Envelope($message, [new DelayHeader($delay)]);
$this->bus->dispatch($e);
// Applicative log in database that a message has been sent. I naively expect $user to still be available and known from the entity manager
$this->userLogger->userLog($user, 'send_email_command',"sent message with a delay of $delay seconds");now produce the following error message I can workaround it in my application, but I'm not sure this is a desirable feature to have this middleware called when a message is sent. Is there a way to use this middleware only when unning consumer and not when sending message ? Edit: after some more search, a similar discussion goes on #32436 |
|
See #34066 |
fabpot
added a commit
that referenced
this pull request
Oct 22, 2019
…ng always (weaverryan) This PR was merged into the 4.4 branch. Discussion ---------- [Messenger] Fix worker-only Doctrine middleware from running always | Q | A | ------------- | --- | Branch? | 4.4 (or 4.3?, this is a bug fix) | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #32436 Depends on #34069 | License | MIT | Doc PR | not needed Several Doctrine middleware are only meant to be run in a "worker" context: we want to "ping" the connection, "close" the connection and "clear" the entity manager ONLY when we are receiving messages. Before this PR, it was done always, which causes bad behavior for sync messages (imagine your Doctrine connection being closed in the middle of a controller or see #31334 (comment)). This fixes that in a pragmatic way: no new system for "worker-only" middleware or anything like that: just make the middleware smart enough to only do their work when a message is being received async. Commits ------- 290a729 Fixing issue where worker-only middleware were run in all contexts
This was referenced Nov 12, 2019
Merged
Merged
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.
General purpose of this middleware: