Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[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

Merged
merged 1 commit into from
Oct 5, 2021

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented May 18, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR

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.

@lyrixx
Copy link
Member Author

lyrixx commented Jul 23, 2021

ping @symfony/mergers

@lyrixx lyrixx changed the title [Messenger] Add support for DoctrineTransactionWatchdogMiddleware [Messenger] Add a middleware to ensure all transaction has been closed Jul 23, 2021
@lyrixx lyrixx force-pushed the messenger-transation-watchdog branch from e7db3da to d0dbdd0 Compare July 23, 2021 16:24
@lyrixx
Copy link
Member Author

lyrixx commented Jul 23, 2021

@OskarStark Thanks for the review.


I added tests + entry in the CHANGELOG.
It should be OK

@lyrixx lyrixx force-pushed the messenger-transation-watchdog branch from d0dbdd0 to bad5019 Compare July 23, 2021 16:28
@Nyholm
Copy link
Member

Nyholm commented Jul 24, 2021

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 👎

@lyrixx
Copy link
Member Author

lyrixx commented Jul 25, 2021

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.

@lyrixx lyrixx force-pushed the messenger-transation-watchdog branch 4 times, most recently from 230bede to 9929d9c Compare September 1, 2021 17:12
@lyrixx
Copy link
Member Author

lyrixx commented Sep 15, 2021

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.

@lyrixx
Copy link
Member Author

lyrixx commented Sep 22, 2021

Another option would be to throw an exception when the transaction is not finished.

ping @Nyholm ; What do you think ? 👆

@fabpot
Copy link
Member

fabpot commented Sep 22, 2021

First sentence of the description says: "This is a WIP to get some feedback." So, I suppose it cannot be merged yet :)

@lyrixx
Copy link
Member Author

lyrixx commented Sep 22, 2021

@fabpot I have edited the PR description, thanks :)

@chalasr
Copy link
Member

chalasr commented Sep 22, 2021

I share Tobias's feeling here, the proposed middleware seems too specific for being in core to me (same for the exception alternative).

@lyrixx
Copy link
Member Author

lyrixx commented Sep 22, 2021

@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 👼🏼

@chalasr
Copy link
Member

chalasr commented Sep 22, 2021

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.
Could it just log? Or perhaps it could be configurable (log vs exception vs rollback)?

@lyrixx
Copy link
Member Author

lyrixx commented Sep 22, 2021

I could just log indeed, without exception, and without throwing. I'll update the PR.

@lyrixx lyrixx force-pushed the messenger-transation-watchdog branch from 9929d9c to 8a841ce Compare October 1, 2021 09:05
@lyrixx
Copy link
Member Author

lyrixx commented Oct 1, 2021

I updated the PR. now, it only log at error level when a transaction is open

@OskarStark
Copy link
Contributor

Maybe "Detect" is more appropriate than "watchdog"?

@lyrixx lyrixx force-pushed the messenger-transation-watchdog branch from 8a841ce to d9f31ac Compare October 4, 2021 15:35
@lyrixx
Copy link
Member Author

lyrixx commented Oct 4, 2021

I have renamed everything:

Add a middleware to log when transaction has been left open DoctrineOpenTransactionLoggerMiddleware

@lyrixx lyrixx changed the title [Messenger] Add a middleware to ensure all transaction has been closed Add a middleware to log when transaction has been left open DoctrineOpenTransactionLoggerMiddleware Oct 4, 2021
@lyrixx lyrixx changed the title Add a middleware to log when transaction has been left open DoctrineOpenTransactionLoggerMiddleware [Messenger] Add a middleware to log when transaction has been left open Oct 4, 2021
@chalasr chalasr force-pushed the messenger-transation-watchdog branch 2 times, most recently from f454955 to faae1a2 Compare October 5, 2021 14:42
@chalasr chalasr force-pushed the messenger-transation-watchdog branch from faae1a2 to 838846a Compare October 5, 2021 15:04
@chalasr
Copy link
Member

chalasr commented Oct 5, 2021

Thank you @lyrixx.

@chalasr chalasr merged commit 2100d7b into symfony:5.4 Oct 5, 2021
@lyrixx lyrixx deleted the messenger-transation-watchdog branch October 5, 2021 16:15
This was referenced Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.