-
Notifications
You must be signed in to change notification settings - Fork 159
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
refactor: eventPool get atomicity #230
base: main
Are you sure you want to change the base?
Conversation
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.
整体看实现有点复杂。
把event直接用chan作为存储,然后getN()用lock+for select eventChan是不是会简洁一点?类似以下伪代码:
func (p *Pool) GetNContext(ctx context.Context, n int) ([]api.Event, bool) {
p.lock.Lock()
defer p.lock.Unlock()
events := make([]api.Event, 0, n)
for {
select {
case e := <-p.events:
events = append(events, e)
if len(events) == n {
return events, true
}
case <-ctx.Done():
return nil, false
}
}
}
} | ||
for el := p.requestQueue.Front(); el != nil; el = el.Next() { | ||
notify := el.Value.(*request) | ||
if p.free >= notify.N { |
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.
有足够的数量了,直接close(notifty.C)就可以了吧
|
这样实现的话,并发时后续 GetNContext 会被阻塞在 p.lock.Lock() 无法相应 context.Done() |
Proposed Changes:
当前 eventPool 实现在并发调用 GetN 时会导致死锁问题见 #228 。
重构了 eventPool 实现主要变更: