-
Notifications
You must be signed in to change notification settings - Fork 344
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
[Issue 527] fix send goroutine blocked #530
[Issue 527] fix send goroutine blocked #530
Conversation
@@ -490,21 +490,36 @@ func (p *partitionProducer) failTimeoutMessages() { | |||
|
|||
// iterate at most viewSize items | |||
for i := 0; i < viewSize; i++ { | |||
item := p.pendingQueue.Poll() | |||
tickerNeedWaiting := time.Duration(0) | |||
item := p.pendingQueue.CompareAndPoll( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be used to refactor the pendingQueue.Peek()
at line 464
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be used to refactor the
pendingQueue.Peek()
at line 464
@merlimat There is not necessarily. pendingQueue.Peek()` at line 464 is to check the first item of queue whether timeout. It not modify the queue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the bug reason is peek head item and determine if it timeout, and then get a snapshot of pendingQueue, range snapshot and poll all the items in snapshot even if it is not all timeout.
the pendingQueue.Peek() at line 464 just peek(not poll) the head item in queue and check(logically correct), there are no actual operation to pendingQueue, so it looks like no need to refactor
@wolfstudy When this pull request merge to the master branch? |
@merlimat please review my code and merge this pull request to master branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1
cc / @cckellogg @merlimat PTAL |
@jiangbo9510 Please merger master code for failed action CI |
@wolfstudy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jiangbo9510 This change may not really solve the problem
@wolfstudy It's anothor problem. If it occured,All send goroutins will blocked by the dead lock. In my case, Sometimes some send goroutins blocked. and others goroutins can send message successful. |
Fix Issue: #527
Motivation
I add a function to compare and poll block queue in atomic. To fix poll a wrong item in
failTimeoutMessages()
Modifications
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
Documentation