leveldown icon indicating copy to clipboard operation
leveldown copied to clipboard

Synchronous Methods

Open kriszyp opened this issue 7 years ago • 14 comments

I think there can some compelling reasons to offer synchronous versions of the leveldown functions (returning immediately without a callback, not disc sync). Sometimes synchronous can simply be easier to reason about (less race conditions to consider with other code running while waiting). And, in particular with leveldb with small-valued databases, gets and puts are often faster than node's worker/queue mechanism, the queuing of the event callback can take more time than the operation itself.

There has been some prior work to add this functionality here: https://github.com/snowyu/node-nosql-leveldb This is out-of-date and no longer seems to compile. I have been updating/porting some of the synchronous functions in my own branch for our purposes: https://github.com/kriszyp/leveldown

I am just curious if such efforts might be considered for upstream merging? I am fine with just using my own branch for our application, and I haven't really completed a full set of sync methods, I just converted enough functionality for our own purposes, but would there be value in having a full set of synchronous equivalent functions in the main leveldown repo?

And thanks for the great package, this has been incredibly valuable to us!

kriszyp avatar Jun 23 '17 12:06 kriszyp

+1 for openSync, getSync, and putSync. We often use leveldown for simple analytics jobs, atomic ops (like INCR in Redis or findAndModify in MongoDB) could be implemented easier with these synchronous methods.

But as discussed in https://github.com/Level/leveldown/pull/136, do we really need synchronous iterators? The iterator code is quite complicated already 😰.

peakji avatar Jun 23 '17 14:06 peakji

I don't mind adding sync functionality if others find it useful. When implementing tests we should consider updating abstract-leveldown as well so other down implementations can implement their own sync methods.

ralphtheninja avatar Jun 24 '17 02:06 ralphtheninja

+1 for getSync, putSync, delSync. Could massively improve performance for certain use cases.

frankboehmer avatar Aug 21 '17 10:08 frankboehmer

Already update. but only supports above node v4 and remove native async methods instead of js wrapper. I think these thread bugs(eg #157) are fixed which I've added in my test. Please notice me if bugs still there.

snowyu avatar Jun 19 '18 05:06 snowyu

@snowyu Thanks for the reference to the bug.

How can I test your updated code? I'd like to confirm this bug ASAP because recently I meet it in my production environment every day.

Related to https://github.com/Chatie/wechaty/issues/1355

huan avatar Jun 22 '18 05:06 huan

just treat it as leveldown:

import LevelDB from 'nosql-leveldb';
import LevelUp from 'levelup';
levelUp('/tmp/thedbdir', {db: LevelDB}, (err, leveldb)=>{})

snowyu avatar Jun 22 '18 12:06 snowyu

@snowyu Thank you very much. I'm updating my module right now and will get back to you when I have any results.

huan avatar Jun 23 '18 17:06 huan

@snowyu had you tested nosql-leveldb under mac?

I got a Segmentation fault when run test in TravisCI, which worked without any problem with leveldown before.

See: https://travis-ci.com/zixia/flash-store/jobs/130978289

huan avatar Jun 23 '18 17:06 huan

No, Only on Linux. More detail wanted. Did you try the unit test of nosql-leveldb on Mac?

snowyu avatar Jun 24 '18 01:06 snowyu

I had just checked the TravisCI from your repository https://github.com/snowyu/node-nosql-leveldb at:

https://travis-ci.org/snowyu/node-nosql-leveldb/builds/395980328

The unit tests of nosql-leveldb on Mac is all FAILED, could you please fix them?

However there's no Segantation fault, I'll check it later.

huan avatar Jun 24 '18 03:06 huan

Please continue further discussion about nosql-leveldb in its own repo.

vweevers avatar Jun 24 '18 04:06 vweevers

@vweevers Ok.

@snowyu Can you enable Issue for your repository? Becasue you forked it from the leveldown, by default we can not file issue on your repo.

huan avatar Jun 24 '18 05:06 huan

@zixia done.

I've found the reason: the c++11 feature std:mutex raised error on Mac.

snowyu avatar Jun 25 '18 02:06 snowyu

For those interested in an immediate solution: https://github.com/Level/mem/pull/50

ccorcos avatar Feb 28 '19 08:02 ccorcos