Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronous Methods #372

Open
kriszyp opened this issue Jun 23, 2017 · 14 comments
Open

Synchronous Methods #372

kriszyp opened this issue Jun 23, 2017 · 14 comments
Labels
discussion Discussion enhancement New feature or request

Comments

@kriszyp
Copy link

kriszyp commented Jun 23, 2017

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!

@peakji
Copy link
Member

peakji commented Jun 23, 2017

+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 #136, do we really need synchronous iterators? The iterator code is quite complicated already 😰.

@ralphtheninja
Copy link
Member

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.

@frankboehmer
Copy link

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

@snowyu
Copy link

snowyu commented Jun 19, 2018

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.

@huan
Copy link
Contributor

huan commented Jun 22, 2018

@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 wechaty/wechaty#1355

@snowyu
Copy link

snowyu commented Jun 22, 2018

just treat it as leveldown:

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

@huan
Copy link
Contributor

huan commented Jun 23, 2018

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

@huan
Copy link
Contributor

huan commented Jun 23, 2018

@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

@snowyu
Copy link

snowyu commented Jun 24, 2018

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

@huan
Copy link
Contributor

huan commented Jun 24, 2018

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.

@vweevers
Copy link
Member

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

@huan
Copy link
Contributor

huan commented Jun 24, 2018

@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.

@snowyu
Copy link

snowyu commented Jun 25, 2018

@zixia done.

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

@ccorcos
Copy link

ccorcos commented Feb 28, 2019

For those interested in an immediate solution: Level/mem#50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants