-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Fix issue where commenting with a status change could partially fail. #35317
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
When commenting on an issue while changing its status (close or reopen), the comment may sometimes not be created even though the status has changed. This happens because the two operations are executed in separate database transactions.
Are you sure it is really the problem? In the defer func(), the previous transaction should have been committed, so where is the bug?
When commenting on an issue while changing its status (close or reopen), the comment may sometimes not be created even though the status has changed. This happens because the two operations are executed in separate database transactions.
Are you sure it is really the problem? In the
defer func(), the previous transaction should have been committed, so where is the bug?
I’m certain there’s a bug here—I’ve encountered it multiple times myself. If CreateComment fails, the defer functions are still executed, but there’s no err check in place.
I believe this change is wrong. The defer func() itself is already an abuse, you didn't figure the root problem but make the logic more messy.
I believe this change is wrong. The
defer func()itself is already an abuse, you didn't figure the root problem but make the logic more messy.
The issue occurs because createComment fails, but the close/reopen action still continues executing. I have mentioned this in my previous comment. I think you are right, the defer function looks not right. I have sent 0434311 to remove it.
Then it comes to another question: why you need to change issue_service.CloseIssue/ReopenIssue (add more arguments like "", nil)?
I think the only thing you need to do is to rewrite the abused defer func()?
Then it comes to another question: why you need to change
issue_service.CloseIssue/ReopenIssue(add more arguments like"",nil)?I think the only thing you need to do is to rewrite the abused
defer func()?
Because Comment and Close should be in the same database transaction to ensure it will not fail partially. Or I can create a new method named ComemntAndClose and ComentAndReopen. I don't think rewrite the defer func() could address the target.
...ea into lunny/fix_comment_close_reopen
...ea into lunny/fix_comment_close_reopen
When commenting on an issue while changing its status (close or reopen), the comment may sometimes not be created even though the status has changed. This happens because the two operations are executed in separate database transactions.
This PR ensures both operations are performed within the same database transaction, preventing such inconsistencies.