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

context for commands (deadline / cancellation) #47

Open
abustany opened this issue May 14, 2020 · 4 comments
Open

context for commands (deadline / cancellation) #47

abustany opened this issue May 14, 2020 · 4 comments

Comments

@abustany
Copy link

Is there a plan (or wish?) to add APIs for Put etc that would take a context.Context in, and handle cancellation/deadlines? Put another way, how would the current API deal with (for example) stalled connections?

@kr
Copy link
Member

kr commented May 15, 2020

That's a good question. At first glance, I'm not sure how useful it'd be, because the protocol doesn't have any way to cancel requests (other than to close the connection). So in the case where a deadline is exceeded or the context is explicitly canceled, there is a question of what this package should do: should it close the connection, or perhaps leave the canceled request pending? Both of those have significant drawbacks. It is not obvious to me what the "correct" behavior should be.

@kr
Copy link
Member

kr commented May 15, 2020

One thing we could do pretty easily would be to convert a context's deadline into a timeout for a reserve command, but there is still no way in the protocol to cancel a reserve command if its context is explicitly canceled.

@abustany
Copy link
Author

Hmm yes you have a point about not being able to cancel requests at the protocol level... But then I guess this is more generally true for any non-idempotent request with for example an HTTP request, and go's HTTP client still lets you cancel those using the context.

The situation I have in mind is a service with a given time budget (SLA), which needs to enqueue a job: with the current API, ensuring that my Put call returns in time is a bit tricky. I guess I'd need to use a custom dialer that calls SetWriteDeadline on the connection to ensure the read/write calls don't stall for too long (but setting the deadline at the conn level ignoring the protocol semantics might end up break things?). Leaving the decision of what to do in case of a timeout to the application designer does not seem like a crazy idea to me: I guess if I need to be absolutely sure that a given job is not enqueued twice I can put protections in place for that?

I don't have experience using beanstalkd in production (yet :-) ) so I might be missing something completely here, any input from more seasoned users is welcome!

@kr
Copy link
Member

kr commented May 18, 2020

I think idempotence is not the main problem. Non-idempotent HTTP requests can be canceled.

The problem here is that the only way to cancel a beanstalkd request is to close the connection, and that can affect other commands that the application might not have wanted to cancel. So I think that is not a good approach for this general-purpose client library. (But, as you point out, it would be reasonable for an application to set a read deadline on the connection, to produce a similar effect.)

The other way this package might handle a canceled context is to abandon the command. It could return to the call site early so the application can continue, while leaving the connection open and leaving the command pending. This package would still internally have to wait for and read the response, so that subsequent commands still work.

I think that would be reasonable. An application can get that effect today by doing something like this:

var id uint64
var err error
completed := make(chan struct{})

go func() {
    id, err = conn.Put(…)
    close(completed)
}()

select {
case <-completed:
    // use id and err…
case <-ctx.Done():
    // canceled or timed out…
}

If we added context support to go-beanstalk, the implementation would probably look a lot like this.

Note that this approach introduces concurrency around the arguments to Put, and that would probably still be true even with a version baked in to this package. That might be a surprising pitfall. We should think about it carefully.

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