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

[Go] Studying an approach for integration with context.Context #7198

Open
gdm85 opened this issue May 19, 2022 · 3 comments
Open

[Go] Studying an approach for integration with context.Context #7198

gdm85 opened this issue May 19, 2022 · 3 comments

Comments

@gdm85
Copy link

gdm85 commented May 19, 2022

Hi there,

I am new to FoundationDB and was looking around in the issue tracker for closed issues/PRs about this topic but could not find any.

I have been reading also this interesting conversation from 2014: https://groups.google.com/g/golang-nuts/c/SPjQEcsdORA

My question is: how would one integrate Go's context.Context when using the Go binding of FoundationDB? I can imagine that the idea has already been explored; ideally I could contribute with some documentation/example/PR afterwards.

I mostly wonder if it would require a re-hash of this old approach for the waiting mechanisms (most likely not): https://gist.github.com/ipeters/4bc867c5e093679ab145
and what guarantees can be kept once context.Context is handled.

An example of the use case I am referring to:

package main

import (
	"fmt"
	"log"
	"net/http"

	"github.com/apple/foundationdb/bindings/go/src/fdb"
)

var db fdb.Database

func hello(w http.ResponseWriter, req *http.Request) {

	// USE-CASE: let's cancel 'db.Transact' when req.Context() is cancelled

	ret, e := db.Transact(func(tr fdb.Transaction) (interface{}, error) {
		tr.Set(fdb.Key("hello"), []byte("world"))
		return tr.Get(fdb.Key("foo")).MustGet(), nil
	})
	if e != nil {
		log.Fatalf("Unable to perform FDB transaction (%v)", e)
	}

	fmt.Fprintf(w, "hello is now world, foo was: %s\n", string(ret.([]byte)))
}

func main() {
	fdb.MustAPIVersion(720)
	db = fdb.MustOpenDefault()

	http.HandleFunc("/hello", hello)
	http.ListenAndServe(":8090", nil)
}
@jzhou77
Copy link
Contributor

jzhou77 commented May 23, 2022

@alecgrieser and @johscheuer , do you know the answer to this question?

@alecgrieser
Copy link
Contributor

No, not really, though I'm not all that familiar with the go bindings, or with go's http stack. If there is a way of inserting a callback on the request context that runs when the context is cancelled, it might be enough to cancel (https://pkg.go.dev/github.com/apple/foundationdb/bindings/go/src/fdb#Transaction.Cancel) the transaction, though as the documentation notes:

If your program attempts to cancel a transaction after (Transaction).Commit has been called but before it returns, unpredictable behavior will result. While it is guaranteed that the transaction will eventually end up in a cancelled state, the commit may or may not occur.

Which of course could be a problem for some use cases.

There are also concerns with concurrent calls to Reset, which I don't see in the relevant implementation of Transact:

func (d Database) Transact(f func(Transaction) (interface{}, error)) (interface{}, error) {

That being said, it's possible that OnError (which is called) might have similar problems, as that (I thought) called reset under the hood, but I could be wrong there. So the call back may need to do something more clever, like rely on a mutex or something that is also acquired prior to the transaction being committed...or something.

@gdm85
Copy link
Author

gdm85 commented May 25, 2022

If there is a way of inserting a callback on the request context that runs when the context is cancelled, it might be enough to cancel (https://pkg.go.dev/github.com/apple/foundationdb/bindings/go/src/fdb#Transaction.Cancel) the transaction

That is not possible with context.Context own features but the simplest way to do it would be to have a separate goroutine, as in:

package main

import (
	"context"
	"fmt"
	"log"
	"net/http"

	"github.com/apple/foundationdb/bindings/go/src/fdb"
)

var db fdb.Database

func autoCancel(ctx context.Context, tr fdb.Transaction) <-chan struct{} {
	exitCh := make(chan struct{})
	go func() {
		select {
		case <-exitCh:
			return
		case <-ctx.Done():
			tr.Cancel()
			return
		}
	}()
	return exitCh
}

func hello(w http.ResponseWriter, req *http.Request) {

	// USE-CASE: let's cancel 'db.Transact' when req.Context() is cancelled

	ret, e := db.Transact(func(tr fdb.Transaction) (interface{}, error) {
		defer close(autoCancel(req.Context()))

		tr.Set(fdb.Key("hello"), []byte("world"))
		return tr.Get(fdb.Key("foo")).MustGet(), nil
	})
	if e != nil {
		log.Fatalf("Unable to perform FDB transaction (%v)", e)
	}

	fmt.Fprintf(w, "hello is now world, foo was: %s\n", string(ret.([]byte)))
}

func main() {
	fdb.MustAPIVersion(720)
	db = fdb.MustOpenDefault()

	http.HandleFunc("/hello", hello)
	http.ListenAndServe(":8090", nil)
}

A better version perhaps would be to have the channels select lower in the API, like it was done originally here: https://gist.github.com/ipeters/4bc867c5e093679ab145

But either would need some testing to find out if there are nasty surprises.

Would it be beneficial if I make a PR that introduces a ctx context.Context to the constructor of futures? We can discuss there in the PR how this change should reflect in current API and whether to use something like the approach above or a more efficient one that would nest here:

as the documentation notes:

If your program attempts to cancel a transaction after (Transaction).Commit has been called but before it returns, unpredictable behavior will result. While it is guaranteed that the transaction will eventually end up in a cancelled state, the commit may or may not occur.

Yes, these are hard facts and I think they will reflect in this way on any API built on top:

  • if you call the API and receive a positive response then state was changed
  • if you cancelled and/or timed out and never received a response, you do not know whether state was changed or not

I think this matches what the current Go documentation says and it must be true in general for any API changing state over network as there is no way to know remote state (without querying) in case of any network issue.

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

3 participants