-
Notifications
You must be signed in to change notification settings - Fork 535
back out locking added in c78ada09f230a3c66cd934860700f93ff31a93eb #764
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,23 +19,19 @@ package execpool | |
| import ( | ||
| "context" | ||
| "sync" | ||
|
|
||
| "github.com/algorand/go-deadlock" | ||
| ) | ||
|
|
||
| // A backlog for an execution pool. The typical usage of this is to | ||
| // create non-blocking queue which would get executed once the execution pool is ready to accept new | ||
| // tasks. | ||
| type backlog struct { | ||
| mu deadlock.Mutex | ||
| pool ExecutionPool | ||
| wg sync.WaitGroup | ||
| buffer chan backlogItemTask | ||
| ctx context.Context | ||
| ctxCancel context.CancelFunc | ||
| owner interface{} | ||
| priority Priority | ||
| quit bool | ||
| } | ||
|
|
||
| type backlogItemTask struct { | ||
|
|
@@ -82,25 +78,11 @@ func (b *backlog) GetParallelism() int { | |
|
|
||
| // IsFull test to see if the input buffer is full. | ||
| func (b *backlog) IsFull() bool { | ||
| b.mu.Lock() | ||
| defer b.mu.Unlock() | ||
| return len(b.buffer) == cap(b.buffer) | ||
| } | ||
|
|
||
| // Enqueue enqueues a single task into the backlog | ||
| func (b *backlog) Enqueue(enqueueCtx context.Context, t ExecFunc, arg interface{}, priority Priority, out chan interface{}) error { | ||
| b.mu.Lock() | ||
| defer b.mu.Unlock() | ||
| if b.quit { | ||
| select { | ||
| case <-enqueueCtx.Done(): | ||
| return enqueueCtx.Err() | ||
| case <-b.ctx.Done(): | ||
| return b.ctx.Err() | ||
| default: | ||
| return nil | ||
| } | ||
| } | ||
| select { | ||
| case b.buffer <- backlogItemTask{ | ||
| enqueuedTask: enqueuedTask{ | ||
|
|
@@ -120,18 +102,6 @@ func (b *backlog) Enqueue(enqueueCtx context.Context, t ExecFunc, arg interface{ | |
|
|
||
| // Enqueue enqueues a single task into the backlog | ||
| func (b *backlog) EnqueueBacklog(enqueueCtx context.Context, t ExecFunc, arg interface{}, out chan interface{}) error { | ||
| b.mu.Lock() | ||
| defer b.mu.Unlock() | ||
| if b.quit { | ||
| select { | ||
| case <-enqueueCtx.Done(): | ||
| return enqueueCtx.Err() | ||
| case <-b.ctx.Done(): | ||
| return b.ctx.Err() | ||
| default: | ||
| return nil | ||
| } | ||
| } | ||
| select { | ||
| case b.buffer <- backlogItemTask{ | ||
| enqueuedTask: enqueuedTask{ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. github won't let me comment on lines not in this PR, but it looks like this doesn't totally undo the changes done to this file in #700 b/c of the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (in both
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may actually be the minimum change needed to fix things. I think without this Enqueue() and EnqueueBacklog() will panic if Shutdown() has been called because they try to send on a closed b.buffer . The internal Context b.ctx is the only thing that can signal that Enqueue shuoldn't actually try to append to b.buffer
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a race between context cancellation and closing the buffer? |
||
|
|
@@ -151,9 +121,6 @@ func (b *backlog) EnqueueBacklog(enqueueCtx context.Context, t ExecFunc, arg int | |
|
|
||
| // Shutdown shuts down the backlog. | ||
| func (b *backlog) Shutdown() { | ||
| b.mu.Lock() | ||
| defer b.mu.Unlock() | ||
| b.quit = true | ||
| b.ctxCancel() | ||
| close(b.buffer) | ||
| b.wg.Wait() | ||
|
|
||
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.
Is this method actually called anywhere?