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

Possible pipeline.Discard bug #205

Closed
derekperkins opened this issue Jan 2, 2023 · 1 comment
Closed

Possible pipeline.Discard bug #205

derekperkins opened this issue Jan 2, 2023 · 1 comment
Assignees

Comments

@derekperkins
Copy link
Contributor

At the beginning of the Discard() function, it checks to see if the context has been canceled and errors out if it has

olric/pipeline.go

Lines 441 to 448 in 4076aef

func (dp *DMapPipeline) Discard() error {
select {
case <-dp.ctx.Done():
return ErrPipelineClosed
default:
}
dp.cancel()

However the same context is canceled during Exec(), so Discard() will always return an error afterwards, which is its primary use case

olric/pipeline.go

Lines 405 to 416 in 4076aef

// Exec executes all queued commands using one client-server roundtrip.
func (dp *DMapPipeline) Exec(ctx context.Context) error {
select {
case <-dp.ctx.Done():
return ErrPipelineClosed
default:
}
dp.mtx.Lock()
defer dp.mtx.Unlock()
defer dp.cancel()

I think what needs to happen is to have two separate contexts / channels:

  1. Used as it is today to block in FutureXXX to wait for Exec to run
  2. A second that is closed inside of Discard and Close that marks all FutureXXX from that pipeline as invalid, rather than potentially returning invalid data / panicking as they would today
@derekperkins
Copy link
Contributor Author

Fixed by #206

@buraksezer buraksezer self-assigned this Jan 2, 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

No branches or pull requests

2 participants