search

npgall / cqengine

Ultra-fast SQL-like queries on Java collections
Apache License 2.0
1.72k stars 252 forks source link

Question: change OffHeapPersistence's readWriteLock acquire mode to tryLock ? #173

Open tylaar opened 6 years ago

tylaar commented 6 years ago

Hi Niall,

Sorry to bother again. We've been trying to use OffHeapPersistence mode a lot these days, and found something that would like to ask.

Currently OffHeapPersistence's readWriteLock is directly using "connectionLock.lock()", while actually if there is bug in client code that forget to release readLock in retrieve procedure, it could make the writer been blocked in silence. It won't be a problem of course in single thread or main thread application mode, but it would take client side a lot of time to debug in a multithread application. Since it doesn't throw Exception also, user won't notice the problem until they use jstack or other profiler tools to check.

So my question is, is that possible to change OffHeapPersistence's lock method to use tryLock(long time, TimeUnit unit), and let client side code handle the lock not acquired scenario? It would provide a fail fast way I think.

tylaar commented 6 years ago

Any idea on this Niall ?

npgall commented 6 years ago

Hi Tylaar,

Sorry for the delay replying.

For now, your best option is probably to subclass OffHeapPersistence and override the getConnection() method to use tryLock() instead of lock().

I will try to make it easier for the application to inject a custom ReadWriteLock in future though - as an alternative to having to override the getConnection() method. For example it could inject a lock which transparently applies the timeout.

I will keep this issue open as a reminder to myself to do that.

tylaar commented 6 years ago

Would you mind if I have a try? Maybe I can help on this by submit a PR stuff :-)

npgall commented 6 years ago

Yes please do!

It might make sense to have the constructor of OffHeapPersistence accept a QueryOptions object as well as the Properties object - in this instance to allow a custom lock to be passed in via QueryOptions. And then maybe add static methods such as onPrimaryKeyWithOptions(..) and onPrimaryKeyWithPropertiesAndOptions(..).

QueryOptions are used elsewhere in the library to allow optional objects or flags to be passed around without requiring option-specific API changes. The idea being that as long as an API accepts QueryOptions then the API won't need to be changed thereafter no matter what options we want to supply.

Right now the Persistence objects don't accept QueryOptions - but maybe they should!