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

[Fix] Fix the dispatcher() stuck caused by availablePermitsCh #875

Merged
merged 2 commits into from
Oct 31, 2022

Conversation

Gleiphir2769
Copy link
Contributor

@Gleiphir2769 Gleiphir2769 commented Oct 28, 2022

Motivation

The availablePermitsCh may cause the dispatcher stuck.

case messageCh <- nextMessage:
// allow this message to be garbage collected
messages[0] = nil
messages = messages[1:]
pc.availablePermitsCh <- permitsInc
case pr := <-pc.availablePermitsCh:
switch pr {
case permitsInc:
pc.increasePermitsAndRequestMoreIfNeed()
case permitsReset:
pc.availablePermits = 0
}

For example, if messageCh <- nextMessage continueously selected 10 times, the availablePermitsCh will be filled. The next messageCh <- nextMessage will be stuck forever because pr := <-pc.availablePermitsCh can never be reached.

Modifications

Remove the pc.availablePermitsCh from dispatcher.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

@Gleiphir2769
Copy link
Contributor Author

Gleiphir2769 commented Oct 31, 2022

@RobertIndie @nodece Hi, could you give a review?

@Gleiphir2769 Gleiphir2769 changed the title [Fix] Fix the stuck caused by availablePermitsCh [Fix] Fix the dispatcher() stuck caused by availablePermitsCh Oct 31, 2022
@RobertIndie RobertIndie merged commit 5679f1f into apache:master Oct 31, 2022
@RobertIndie RobertIndie added this to the v0.10.0 milestone Mar 27, 2023
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.

None yet

3 participants