Confusing error when using other Google APIs

[ghost]

Symptom: I tried to use stock Google libraries to access other Google APIs, like Google Storage, but got bit by an obscure error "not an App Engine context". Here's a repro, and a commented out fix I eventually found after talking to @danjacques :

package lucigaeissue

import (
    "net/http"

    "golang.org/x/oauth2/google"
    storage "google.golang.org/api/storage/v1"

    // "github.com/luci/gae/impl/prod"
    "github.com/luci/luci-go/appengine/gaemiddleware"
    "github.com/luci/luci-go/common/logging"
    "github.com/luci/luci-go/server/router"
)

func init() {
    r := router.New()
    mw := gaemiddleware.BaseProd()
    gaemiddleware.InstallHandlers(r, mw)
    r.GET("/", mw, handler)

    http.DefaultServeMux.Handle("/", r)
}

func handler(c *router.Context) {
    scope := storage.DevstorageFullControlScope
    ctx := c.Context // What I'd expect to use
    // ctx := prod.AEContext(c.Context) // What I should really use
    client, _ := google.DefaultClient(ctx, scope)
    service, _ := storage.New(client)
    if _, err := service.Buckets.List("project-id").Do(); err != nil {
        logging.Errorf(c.Context, "Buckets.List failed: %s", err)
    } else {
        logging.Infof(c.Context, "Buckets.List succeeded")
    }
}

What happens: router.Handle() starts the handler with an empty context;
gaemiddleware.BaseProd() installs ProdServices which calls prod.Use which hides the true AppEngine context under a key.

As a result, it appears that c.Context is an App Engine context, because Datastore and other GAE services work just fine, yet calling a native Cloud API fails with "not an App Engine context".

This error is very non-intuitive, and cost me hours of debugging. Turns out, I simply needed to use prod.AEContext(c.Context) in DefaultClient(). I did not find any mention of it in the documentation, not even for AEContext itself.

The problem stems from the fact that c.Context appears to be the only way to get a context.Context value, and nothing indicates that it's not an App Engine context.

So, it's a UX / useability issue which could probably be fixed by better documentation, or simply allowing c.Context to be a derivative of the actual App Engine context. The latter might be a luci-go issue.

Thanks!

[riannucci]

Some background: The reason that it's hidden under a key is because otherwise it would be possible to mix and match luci/gae invocations with upstream datastore invocations. Doing this could lead to confusing results at best, and dataloss at worst.

Unfortunately we don't have a good way to intercept these other usages of the actual AEContext to improve this error message, so documentation (maybe an FAQ?) is probably our best bet.

This may be related to #23, but that's not entirely clear.

[vadimsht]

I've also hit this with PubSub gRPC client. It needs real GAE context to be able to use Sockets API from inside gRPC guts.

In gRPC case the error message is even more mysterious: the dialing just times out.

[danjacques]

One option that makes me sad but is maybe less hacky than this:

1. Create a Context implementation that records all fetched keys.
2. Using that Context, execute a targeted native AppEngine call, cache all keys that are referenced via Value.
3. Wrap GAE's returned Context in a layer whose Value craps out if one of those keys is referenced.

[danjacques]

That said, that is a way of blocking GAE calls without having a separate AEContext. It is not a way to combine the Contexts.

[riannucci]

Also proposed: don't hide it any more. If users mix+match luci/gae with datastore/taskqueue native APIs, they'll suffer the consequences. That said, it may be possible for us to wrap the AEContext with a layer like the one dnj proposed, and panic if any calls are made via the native APIs (somehow... not sure precisely how to do this, but I think it could be done). That would provide the best of all worlds:

• Panic for bad API usage (with a clear error message) instead of data corruption
• No need for AEContext functions (just use the Context directly).

[ghost]

Thanks for the background! Besides betters docs, I don't know if this really needs a fix, unless we can indeed come up with a good error / panic on invalid usage.

One of the reasons for filing this issue is to surface it in Google search, sorta like stackoverflow-on-github.

[danjacques]

Another option is to just get rid of AEContext as a public function and make it an internal implementation detail. Users who want their own can create their own manually using the AppEngine APIs, maybe on top of the existing Context (preserving logging etc.).

We could facilitate this by exposing the request via impl/prod API.

[riannucci]

@danjacques I think that would have still lead to exactly this bug report, except without a solution :)

[danjacques]

It would, but it would also avoid the dual Context problem.

[ghost]

I'm with @riannucci on this one - this is a usability problem, and it won't improve by removing features. A solution would ideally be immediately obvious from the error, and less ideally be clearly documented in visible places.

[bsr203]

good timing :-) hit the same while using search API