Segfault with subleveldown #60
Comments
|
Are you sure the following commands are correct? |
|
Oof, sorry about that. I've double-checked that this is 100% correct: git clone https://github.com/flumedb/flumeview-level
cd flumeview-level
git checkout -b sub
git pull origin sub
npm ci
node test/read.js |
|
I personally get a
It's probably the same issue but blows up somewhere else due to timing etc. It's difficult to see directly where this goes wrong. My gut feeling tells me something is trying to read data after the database has been closed. This is (unfortunately) a known problem with |
|
Btw, is this test code new or has it been around for a while? Wondering if there's something that has changed in |
|
@vweevers Any ideas? |
|
Test code is super new. I'm on mobile so this will be brief, but the gist is that on Scuttlebutt we have many instances of leveldown running and I wanted to experiment with a single instance shared by subleveldown. As far as I can tell this passes all previous tests except "retest", which tries to close and re-open to database to ensure persistence. Is my intuition correct that sharing an instance of leveldown via subleveldown may be more performant, or is this all just a wild goose chase? Thanks for your help! I'm really appreciative at how quickly you've triaged this issue. |
This is good to know.
It might be more performant. You'd have to write some benchmarks to verify this. Also depends on what type of performance you're after. Maybe lower memory usage at the expense of more key processing? Just guessing here. Anyway. I think you should take a closer look at the code dealing with closing and re-opening the database. Take care to close the database once you know all iterators are done (read streams etc), then re-open. |
|
On Windows I get a nullptr in a
|
|
@christianbundy I think the issue (or at least one issue) is that the database isn't actually closed. Because |
|
You can check this by adding the following lines to db.on('closing', () => console.error('closing'))
db.on('closed', () => console.error('closed'))Both don't happen, until I make the following patch to index ca81863..e77e925 100644
--- a/index.js
+++ b/index.js
@@ -179,8 +179,11 @@ module.exports = function (log, isReady, mapper) {
if(sv.close) sv.close(cb)
else cb()
}
- })) (cb)
-
+ }))(function (err) {
+ log.db.close(function (err2) {
+ cb(err || err2)
+ })
+ })
}
}
return flume |
|
Side note: |
This leads me to think it hasn't been opened since |
|
@ralphtheninja with the |
|
@christianbundy Do you have enough information to continue? |
|
Yeah, it sounds like I'm trying to open an already-open database, but I'm not clear on why that's segfaulting rather than just throwing an error. I've tried to write a minimal reproducible test-case, but I'm not sure exactly why this segfaults. I'm happy to just use the workaround of not trying to open an already-open database, but I was surprised by the segfault. |
|
True, the segfault does warrant further investigation. I'll have a look. |
|
@christianbundy I'm struggling to write a test case (I can't seem to produce a segfault unless I create an iterator, but that's not the issue here AFAICT and already slated to be fixed by #597). Could you share yours? |
|
Sorry, I mean that I've also been unable to write a minimal test case. You can still use these instructions but I haven't had the time to whittle it down any further. |
|
It seems ralphtheninja was on the right track in https://github.com/Level/leveldown/issues/590#issuecomment-459952479. The chain of db's is not closed properly somewhere, leading to a broken state. Then when reopened, which wraps some of the existing db's in new db's, the underlying Initially, the state is:
Once opened, all those statuses change to
Note that Then, in the
Because
|
|
So Hard to explain, but I think I have enough info now to write a test. |
|
Also, the reason that the close patch above (https://github.com/Level/leveldown/issues/590#issuecomment-459950304) fixes the situation, is because in that case, the original |
|
Long story short, the bug is in
I prefer 1, but feel like I'm missing something because the way it currently works suddenly seems needlessly complicated. AFAICT Moving this issue to the |
Ah yes. I was missing the fact that the |
|
@christianbundy With Could that be because (The "empty close" tests also still fail but to reiterate, that's a separate problem and I want to put that aside for now) |
|
No, these tests never passed -- this was an experiment with adding subleveldown, and when I got a segfault I just reported with my test-case. It's very possible that it's writing unprefixed data, I don't think that I understood that a prefix was necessary so that's probably been implemented incorrectly by me. |
|
OK. FYI, it might take some time for us to find a proper fix for the subleveldown bug. Also, don't use that |
|
@christianbundy Why did you close this? Did you find a fix on your end? |
DeferredOpen means that the db opens itself and defers operations until it's open. Currently that's only supported by level(up) and friends. Before, subleveldown would also accept abstract-leveldown db's that were not wrapped in levelup. Opening and closing a sublevel no longer opens or closes the parent db. The sublevel does wait for the parent to open (which in the case of levelup already happens automatically) but never initiates a state change. Drops support of old modules: - memdb (use level-mem instead) - deferred-leveldown < 2.0.0 (and thus levelup < 2.0.0) - abstract-leveldown < 2.4.0 Closes #84, #83 and #60.
DeferredOpen means that the db opens itself and defers operations until it's open. Currently that's only supported by level(up) and friends. Before, subleveldown would also accept abstract-leveldown db's that were not wrapped in levelup. Opening and closing a sublevel no longer opens or closes the parent db. The sublevel does wait for the parent to open (which in the case of levelup already happens automatically) but never initiates a state change. Drops support of old modules: - memdb (use level-mem instead) - deferred-leveldown < 2.0.0 (and thus levelup < 2.0.0) - abstract-leveldown < 2.4.0 Closes #84, #83 and #60.
DeferredOpen means that the db opens itself and defers operations until it's open. Currently that's only supported by levelup (and levelup factories like level). Previously, subleveldown would also accept abstract-leveldown db's that were not wrapped in levelup. Opening and closing a sublevel no longer opens or closes the parent db. The sublevel does wait for the parent to open (which in the case of levelup already happens automatically) but never initiates a state change. If one closes the parent but not the sublevel, subsequent operations (like get and put) on the sublevel will yield an error, to prevent segmentation faults from underlying stores. Drops support of old modules: - memdb (use level-mem instead) - deferred-leveldown < 2.0.0 (and thus levelup < 2.0.0) - abstract-leveldown < 2.4.0 Closes #84, #83 and #60.
Disclaimer: this is almost definitely my fault. I'm completely unfamiliar with leveldown and I'm probably doing everything wrong.
With that out of the way, I have a reproducible segfault while using subleveldown that's highly correlated with open/close events.
git clone https://github.com/flumedb/flumeview-level cd flumeview-level git checkout -b sub git pull origin sub npm ci node test/read.jsHere's the output:
And of course:
Please let me know if there's anything else I can do to help debug this. Thanks a lot! I've been really enjoying my experience with leveldown so far.
The text was updated successfully, but these errors were encountered: