NDB cache management is not thread-safe

[svet-b]

Situation:
I have a single pyroute2 NDB object that's shared between threads. Each thread queries the ndb.neighbours table to find the IP of a host with a particular MAC address (or vice versa). Different threads sometimes run concurrently.

Problem:
Directly after a query is executed, a cache cleaning operation takes place - per https://github.com/svinota/pyroute2/blob/0.5.9/pyroute2/ndb/main.py#L322. Occasionally two or more threads will attempt to simultaneously remove the same cache key. The first thread succeeds, but others will throw a KeyError exception, since the cache entry they are trying to remove no longer exists.

Potential solution:
While I'm not very familiar with the workings of the NDB cache, a simple/harmless solution in this case would probably be to catch and ignore a KeyError exception on a cache deletion operation. Not sure if there are wider implications for the thread-safety of NDB though.

I can throw together some reproducer code if helpful, though I hope the issue I'm describing should be fairly clear.

[svinota]

Thanks for the report! Yep, I'm to fix it asap

[svinota]

The first, but most probably not the last fix for the cache management. Please check if it works for you.

[svet-b]

Thanks for the quick turnaround! Looks like that should do it (though haven't had a chance to test yet).

By the way (and this is off-topic for this issue report), but is there a description somewhere as to the workings/intent of the caching mechanism? I checked out https://docs.pyroute2.org/ndb_objects.html#objects-cache which provides some basic info, but what I'm wondering is the following: in my use case I make repeated (read) queries to ndb.neighbours - sometimes pseudo-concurrently, and sometimes seconds/minutes apart. In the debug log I can see cache objects being repeatedly created, deleted, re-created, etc. Since the deletion happens almost immediately after each query, it doesn't look like the cache objects are re-used across queries (as they might be in a "classic" caching scenario). Which made me wonder what the purpose of the cache is to begin with? Is it primarily for staging changes during a "write" operation? Or does it also play a role during a "read" operation?

Thanks for a really great tool by the way!

[svinota]

The main goal of the cache was to keep an object copy during transaction, that is until .commit(). So different writers may access the same object via dict lookups like ndb.interfaces['eth0'] w/o creation of an additional variable to keep the reference. Thus, these three syntax variants must work with one object and produce the same result:

(ndb
 .interfaces['eth0']
 .set('mtu', 1200)
 .set('state', 'up')
 .add_ip('10.0.0.1/24')
 .commit())

or

ref = ndb.interfaces['eth0']
ref['mtu'] = 1200
ref['state'] = 'up'
ref.add_ip('10.0.0.1/24')
ref.commit()

or

ndb.interfaces['eth0']['mtu'] = 1200
ndb.interfaces['eth0']['state'] = 'up'
ndb.interfaces['eth0'].add_ip('10.0.0.1/24')
ndb.interfaces['eth0'].commit()

The performance question is the next, and I hope to optimise the cache from that perspective as well.

But the first step was to make it working in the natural way, as users could expect it to be — in the beginning it wasn't so, and the dict lookup spawned a new object every time.

[svinota]

And why do we spawn objects at all instead of keeping them all in the memory — in the case of thousands of routes or neighbours it becomes a serious issue to keep all the objects as Python objects. So we keep them in a DB, spawning Python objects upon request.

[svet-b]

Thank you for the explanation @svinota! Appreciate it. And definitely sounds like a good approach

[svinota]

That's it. The default timeout for now is 60 seconds, to change it use config.cache_expire:

from pyroute2 import config

# 10 minutes cache expiration for clean objects
config.cache_expire = 600
# cache an object
print(ndb.interfaces['eth0'])

The next step must be the cache limits

[svet-b]

Thanks a lot for the explanation and the additional info! I've gone ahead and closed this as the original issue indeed seems to be resolved.