Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix intermittent CI failure in EmptyQueue #23753

Merged
merged 9 commits into from
May 4, 2023
Merged

Fix intermittent CI failure in EmptyQueue #23753

merged 9 commits into from
May 4, 2023

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Mar 27, 2023 •
edited by silverwind
Loading

The ordering of the final token causing a close of the queue in this test may be out of sync due to concurrency. Instead just use ensure that the queue is closed when everything expected is done.

Fixes: #23608
Fixes: #23977

@zeripath
Fix intermittent CI failure in EmptyQueue
c6673ad 
The ordering of the final token causing a close of the queue in this test may be out of sync due
to concurrency. Instead just use ensure that the queue is closed when everything expected is done.

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath added type/testing outdated/backport/v1.19 This PR should be backported to Gitea 1.19 labels Mar 27, 2023
@zeripath zeripath added this to the 1.20.0 milestone Mar 27, 2023
@KN4CK3R
Copy link
Member

KN4CK3R commented Mar 28, 2023

CI fail is related

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 28, 2023
@zeripath
Copy link
Contributor Author

Sorry about the race - I forgot that there were two things that were simultaneously writing to that map so assumed it didn't need a lock

@silverwind
Copy link
Member

Still failing it seems.

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 2, 2023
@silverwind
Copy link
Member

#24419 had disabled two queue tests on CI, if they are stable again, remove the disablement.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 2, 2023 •
edited
Loading

The old queue code is unmaintainable now, I have started the rewriting. I believe the queue package can be hugely simplified.

@zeripath
Copy link
Contributor Author

zeripath commented May 3, 2023

The problem in this PR was simple; the atomic int needed to be defined outside of the handler and the count needs to be pushed to the done channel outside of the lock.

@wxiaoguang the queue code complexity is a natural result of the requirements. You're just going to create more bugs and reiterate errors.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 3, 2023
@silverwind
Copy link
Member

Added issue references to OP.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 3, 2023
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 4, 2023
@silverwind silverwind enabled auto-merge (squash) May 4, 2023 00:26
@silverwind silverwind merged commit ad8631c into go-gitea:main May 4, 2023
@GiteaBot
Copy link
Contributor

GiteaBot commented May 4, 2023

I was unable to create a backport for 1.19. @zeripath, please send one manually. 🍵

@GiteaBot GiteaBot added backport/manual No power to the bots! Create your backport yourself! and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels May 4, 2023
@wxiaoguang
Copy link
Contributor

@wxiaoguang the queue code complexity is a natural result of the requirements. You're just going to create more bugs and reiterate errors.

Nope, I never created more bugs. I will finish the rewriting soon.

zjjhot added a commit to zjjhot/gitea that referenced this pull request May 4, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
5c32bab 
* upstream/main: (65 commits)
  Changelog for 1.19.3 (go-gitea#24495) (go-gitea#24506)
  Use Actions for DB & E2E tests (go-gitea#24494)
  Fix intermittent CI failure in EmptyQueue (go-gitea#23753)
  Prevent a user with a different email from accepting the team invite (go-gitea#24491)
  Fix incorrect webhook time and use relative-time to display it (go-gitea#24477)
  Make Issue/PR/projects more compact, misc CSS tweaks (go-gitea#24459)
  Implement Cargo HTTP index (go-gitea#24452)
  Clean up polluted styles and remove dead CSS code (go-gitea#24497)
  Improve pull request merge box when pull request merged and branch deleted. (go-gitea#24397)
  Fix EasyMDE toolbar (go-gitea#24489)
  Enhance stylelint rule config, remove dead CSS (go-gitea#24472)
  Fix api error message if fork exists (go-gitea#24487)
  Add ntlm authentication support for mail (go-gitea#23811)
  Fix test delivery button in repo webhook settings page (go-gitea#24478)
  Add Debian package registry (go-gitea#24426)
  Enable whitespace rendering on selection in Monaco (go-gitea#24444)
  Replace `N/A` with `-` everywhere (go-gitea#24474)
  Fix invite display (go-gitea#24447)
  [skip ci] Updated translations via Crowdin
  replace PR docker dry run in drone with Actions (go-gitea#24475)
  ...

# Conflicts:
#	templates/base/footer_content.tmpl
@silverwind silverwind mentioned this pull request May 4, 2023
Merged
@zeripath zeripath deleted the fix-intermittent-failure-in-EmptyQueue branch May 10, 2023 17:07
@lunny lunny removed the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label Jul 26, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 3, 2023
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 14, 2024 •
edited
Loading

The problem in this PR was simple; the atomic int needed to be defined outside of the handler and the count needs to be pushed to the done channel outside of the lock.

@wxiaoguang the queue code complexity is a natural result of the requirements. You're just going to create more bugs and reiterate errors.

@zeripath Just happen to see this unresolved comment, and the new queue system has been in production for more than 1 years, so reply it: the new code is right, it only uses less than a half of lines (reduces thousands of lines), and it is incredibly stable and no bug ever happens. All users are quite happy.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KN4CK3R KN4CK3R KN4CK3R left review comments

@techknowlogick techknowlogick techknowlogick approved these changes

@silverwind silverwind silverwind approved these changes

Assignees
No one assigned
Labels
backport/manual No power to the bots! Create your backport yourself! lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/testing
Projects
None yet
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.

Intermittent CI Failure TestPersistableChannelUniqueQueue Caught a flaky test:
7 participants
@zeripath @KN4CK3R @silverwind @wxiaoguang @GiteaBot @techknowlogick @lunny