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.

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

Revert "Using atomic instead of mutex and delete scratch slice" #1846

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

newacorn
Copy link
Contributor

@newacorn newacorn commented Aug 26, 2024

Reverts #1833
The new implementation of the workerChan queue has the following issues:

  1. The original implementation used push operations for enqueuing and pop operations for dequeuing, while the new implementation uses unshift operations for enqueuing and shift operations for dequeuing. This difference doesn't align with the original design, although this issue might not be significant on its own.

  2. When cleaning up the workerChan queue, the new implementation does not support pop operations. Instead, it uses unshift operations to inspect whether a workChan is active. As a result, old workChan instances cannot be removed unless the youngest one in the queue has also expired.

The solution for issue one related to enqueuing has led to the problem where the cleanup task in issue two cannot be correctly implemented.

A single-headed unidirectional queue cannot fully replicate the original mutex-based workChan queue. When addressing the workChan queue issue with a lock-free linked list, we may need to consider more aspects carefully.

I am sorry to submitted a pull request opposing the previous one because the potentially buggy code has already been merged into the master branch. We need a better solution to the existing issue before considering merging it back into the master branch.

@erikdubbelboer
Copy link
Collaborator

@NikoMalik what do you think, it was your pull request. Do you think it would be possible to fix this problem with cleanup? Otherwise I think we should indeed revert, the performance difference doesn't have such a big impact as it's only for new connections and doesn't impact performance of keep-alive connections.

@erikdubbelboer
Copy link
Collaborator

@newacorn can you please rebase it on master, then I can merge it.

@newacorn
Copy link
Contributor Author

newacorn commented Sep 7, 2024

@newacorn can you please rebase it on master, then I can merge it.

Have done

@erikdubbelboer erikdubbelboer merged commit 74864cb into valyala:master Sep 10, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants