Tests fail when using nds about nds HOT 14 CLOSED

Strange. nds doesn't mutate the *datastore.Keys in any way and just passes them straight to datastore. Do you know which method is returning the error?

from nds.

The method that is returning the error is nds.Get().

My best guess is that the error is happening at the memcache level, rather than on the datastore level. As I don't see any problems in production, I hope that the problem is more to do with how memcache is simulated in the App Engine test environment. The fact that a targeted test passes, indicates to me that it's something to do with a buffer, which would also point the finger at the App Engine test environment. Although I'd like to rule out nds before trying to escalate this with Google.

from nds.

At least the fail can be reproduced so we will be able to get to the bottom of it! Are you able to surround a couple of parts in nds with some printouts to see what the key actually is when it returns a datastore: invalid key?

How about here:

for i, key := range keys {
    if key == nil {
        isNilErr = true
	nilErr[i] = datastore.ErrInvalidKey
        fmt.Println("key was nil") // Add this bit.
    }
}

And here:

go func(i int, keys []*datastore.Key, vals reflect.Value) {
    if _, ok := transactionFromContext(c); ok {
        errs[i] = datastoreGetMulti(c, keys, vals.Interface())
    } else {
        errs[i] = getMulti(c, keys, vals)
    }
    wg.Done()
    // Add this bit.
    if errs[i] == datastore.ErrInvalidKey {
        fmt.Printf("key was invalid %+v\n", errs[i])
    }
}(i, keys[lo:hi], v.Slice(lo, hi))

That way we will hopefully be able to see what the invalid key is.

from nds.

I did that and I'm getting a key was nil message.

I did some further checking in my code to find out where it was possible that a nil key was being returned, and it's in this section of code:

func KeysOnlyFindBy(ctx context.Context, kind, field string, value interface{}) (*datastore.Key, error) {
  if kind == "" || field == "" {
    err := fmt.Sprintf("#n4a6 Kind or field value empty. Kind: %+v. Field: %+v", kind, field)
    log.Criticalf(ctx, "%s", err)
    return nil, errors.New(err)
  }
  // Create a Query object and filter it by fields that are = the value.
  qry := datastore.NewQuery(kind).Filter(field+"=", value).KeysOnly().Limit(1)
  iter := qry.Run(ctx)

  // Return the key and an error if it existed
  key, err := iter.Next(nil)
  if err == datastore.Done {
    return nil, datastore.ErrNoSuchEntity
  }
  return key, nil
}

It turns out that iter.Next(nil) is returning Post http://localhost:56737:EOF as an error, which seems to be connected to the App Engine local server used for testing. I can't find any information on any expected errors from an iter.Next() call, but I'll assume they happen on occasion in production.

It must be incidental that this problem doesn't occur when using nds, as I can't see any logical place where nds could be causing this. However I'm not sure how to report this to the datastore team, as it doesn't happen when using the datastore package directly.

from nds.

Ahh! I believe you have run into one of the most frustrating issues of dev_appserver.py. (Even more frustrating than its slow startup.) It can cause inconsistent test results as you are seeing.

When dev_appserver.pys RPC server is taxed 'too much' for example when the datastore or memcache is being accessed constantly from a 'tight loop' it decides to start returing these weird errors you are seeing.

I believe the reason you see the error with nds and not datastore is because behind the scenes nds also makes RPC calls to memcache, further taxing the dev_appserver.py RPC server. Therefore a unit test that is just on the edge of overloading the dev_appserver.py with datastore might be pushed over with the extra memcache calls nds makes! What's worse is that sometimes these tests will pass depending on your system CPU/disk load and it really doesn't take much to overload the dev_appserver.py.

Workarounds have all been nasty and include putting time.Sleep(time.Second) in tight unit test loops or even in prod code!

However, I have just had a thought, which I am about to try in my code. What if you replaced the appengine.APICallFunc when in dev mode with one that throttles calls:

ctx, closeFunc, err := aetest.NewContext()
if err != nil {
    t.Fatal(err)
}
defer closeFunc()

appengine.WithAPICallFunc(ctx, func(ctx context.Context, 
    service, method string, in, out proto.Message) error {

    // Use a throttling mechanism more refined than this one.
    time.Sleep(500 * time.Millisecond)

    return appengine.APICall(ctx, service, method, in, out)
})

// Run your tests here.

I hope this helps. I'll feedback here if it works for me.

from nds.

The workaround I suggested seems to work well. I have managed to remove all the time.Sleep calls from all my unit tests where I was at risk of getting EOF errors from the dev_appserver.py.

To make it work I created a new package context and replaced appengine.NewContext with context.FromRequest in my code. This allows me to switch in and out of the throttled context with build tags:

$ goapp test -tags=throttle ./...

https://gist.github.com/jongillham/bbc497fa5743e4fcac122f1c93d27039

Maybe there is a better less intrusive way of achieving the same thing.

from nds.

@jongillham I like your solution, but I don't like that we need this solution in the first place.

How do you deal with two "context" packages with imports (i.e. your own context package and the net/context package)? At first glance, it would make more sense to me to use a package with a name other than "context" for this. Any reason you didn't do that?

from nds.

@johngb dev_appserver.py has been a pain. I agree it's horrible that we have to work around the dev environment's shortcomings. The same/similar issue has been reported at least two times a while back:

https://code.google.com/p/googleappengine/issues/detail?id=10854
https://code.google.com/p/googleappengine/issues/detail?id=11086

There are other workarounds. For example https://github.com/luci/gae which has implemented backends in Go for several of GAE's services. I didn't like their API so also had a try with https://github.com/qedus/appengine/tree/master/datastore. (Don't use this though, the API isn't clean.)

As for naming the package context; as you point out it doesn't seem a great idea because of the clash with net/context however I can't think of a better name right now. Hopefully you can think of one and let me know so I can change it! 😀

For the time being I am just doing:

import (
    mypackagectx "github.com/mypackage/context"
    "golang.org/x/net/context"
)

from nds.

I already have an internal project package in the app I'm working on called myctx which handles passing around various bits of data inside a goji web context. So I'll probably just extend myctx to handle this sort of thing. That way once dev_appserver.py gets it's act together, I can easily revert my code to using net/context for everything.

I wish Google would just build the dev app server in Go, but I'm dreaming now...

from nds.

@jongillham Why call the function FromRequest() instead of following the appengine package example of calling it NewContext()?

from nds.

You might be right with NewContext(). context.FromRequest() seemed to describe what was actually happening more clearly than context.NewContext() or context.New(). However convention probably trumps that.

from nds.

@jongillham I'm closing this issue as it's not a nds problem. In any case the solution you suggested is good and works until Google get their act together with dev_appserver.py.

from nds.

Glad I could help. One last thing; I recommend your iterator in KeysOnlyFindBy does this:

key, err := iter.Next(nil)
if err == datastore.Done {
    return key, datastore.ErrNoSuchEntity
}
return key, err // Add this bit.

Just because odd errors can be returned from any of the GAE APIs especially when apps are being automatically transferred between datacentres.

from nds.

I actually implemented those changes while trying to debug the original error (after posting), but I wasn't aware that GAE's API can return errors. Good to know, thanks.

from nds.