-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger] Add a middleware to log when transaction has been left open #41265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ping @symfony/mergers |
src/Symfony/Bridge/Doctrine/Messenger/DoctrineTransactionWatchdogMiddleware.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bridge/Doctrine/Messenger/DoctrineTransactionWatchdogMiddleware.php
Outdated
Show resolved
Hide resolved
e7db3da
to
d0dbdd0
Compare
@OskarStark Thanks for the review. I added tests + entry in the CHANGELOG. |
d0dbdd0
to
bad5019
Compare
Thank you for this PR. This scenario seams like an edge case. My experience is obviously biased, but I can count the number of times I've been working with DB transactions on my one hand. Given you have code in your handler that open a transaction, I think that handler should also be responsible to make sure the transaction is closed. I do see the value for a middleware like this in your application or in a third partly library. But I don't think it should be in the Symfony organisation. Im 👎 |
Don't get me wrong: this middleware is a watchdog. People should not really on it. As you can see, when it's triggered, it generates an error log. So yes, all open transaction must to closed in the handler, I agree with you on this point. |
230bede
to
9929d9c
Compare
People tend to like this feature: https://twitter.com/lyrixx/status/1437440305159094275 and https://twitter.com/lyrixx/status/1418497608658661382 Another option would be to throw an exception when the transaction is not finished. |
ping @Nyholm ; What do you think ? 👆 |
First sentence of the description says: "This is a WIP to get some feedback." So, I suppose it cannot be merged yet :) |
@fabpot I have edited the PR description, thanks :) |
I share Tobias's feeling here, the proposed middleware seems too specific for being in core to me (same for the exception alternative). |
@chalasr as I explained in the PR description, forgetting to close a transaction can really hurt. IMHO this middleware should be native and not op-tin 😬 I understand your point of view, but as you can see, people on twitter tend to like it 👼🏼 |
Well, it's less than your average likes on contrib related tweets isn't it? 🙃 Jokes aside, I can see the potential value for debugging so I won't block this PR. |
I could just log indeed, without exception, and without throwing. I'll update the PR. |
9929d9c
to
8a841ce
Compare
I updated the PR. now, it only log at |
src/Symfony/Bridge/Doctrine/Messenger/DoctrineTransactionWatchdogMiddleware.php
Outdated
Show resolved
Hide resolved
Maybe "Detect" is more appropriate than "watchdog"? |
8a841ce
to
d9f31ac
Compare
I have renamed everything:
|
DoctrineOpenTransactionLoggerMiddleware
DoctrineOpenTransactionLoggerMiddleware
f454955
to
faae1a2
Compare
faae1a2
to
838846a
Compare
Thank you @lyrixx. |
I notice something that can hurt applications: If one open a transaction
in a handler, but don't close it, the transaction is not close by
Doctrine nor Symfony. So all others messages are still handled in the
transaction. More over, when the retry limit is reach, messenger will
try to save the message in the failed transport. In my case it's
doctrine. So the failed message is save, but inner a transaction. When
the worker stop, the transaction is rollbacked, and so the message is
lost.