Closed matthewkeil closed 9 months ago
Interesting! I didn't realize we were this close to supporting this use case.
What's your reasoning for making this opt-in (through allowMultiThreading
)?
Interesting! I didn't realize we were this close to supporting this use case.
What's your reasoning for making this opt-in (through
allowMultiThreading
)?
Mostly so the change was non-breaking for existing users @vweevers. Before I implemented the flag, a few of the unit tests broke so assumed there might be users out there somewhere that are also counting on that behavior. The flag can be stripped out though or I suppose can default to true
There is a workflow issue due to python issue on macos.
Would you like that fixed @vweevers?
Fixed in main: https://github.com/Level/classic-level/commit/3b1a6f27912b486dcd2a010720db30a4891238ee
Would you mind re-running the CI please @vweevers ?
You'll need to rebase your branch to get the fix.
You'll need to rebase your branch to get the fix.
Thanks for the quick fix @vweevers. Rebased!!
The flag can be stripped out though
Yeah. We can do that in a future semver-major version. It would simplify the code a bit, but I think it's good to introduce this feature gradually.
@vweevers I think I got all of your comments addressed. Please let me know if there is anything else that you would like updated.
It might be surprising that with multithreading: false
a handle is still created. E.g. in this example from the tests:
const location = tempy.directory()
const db1 = new ClassicLevel(location)
await db1.open()
t.is(db1.location, location)
One would expect to have exclusive access to this db location. But the code that follows shows the opposite:
const db2 = new ClassicLevel(location)
await db2.open({ multithreading: true })
t.is(db2.location, location)
In other words, I think multithreading: false
should also mean "don't allow others to open this db location". Closer to current behavior.
In other words, I think multithreading: false should also mean "don't allow others to open this db location". Closer to current behavior.
Sounds good!! Updated to achieve that behavior @vweevers. I also added another test case to verify the fix
Nice work! I'll wait with merging for a few days, to give others a chance to review (@juliangruber @ralphtheninja if you're able 🙏).
Hi there @vweevers! Was curious if @juliangruber or @ralphtheninja were interested in taking a bite at the review apple? I will be happy to respond to any comments or concerns.
Aaah! So basically we have a layer inbetween db instances and the corresponding location. Looks good!
Aaah! So basically we have a layer inbetween db instances and the corresponding location. Looks good!
Thanks @ralphtheninja! Much appreciated!! @juliangruber do you have any comments or concerns? @vweevers was curious before merging this... Just a nudge so we can get this imported into the chainsafe/lodestar repo
That'll do.
@vweevers not sure why this unit test failed on the release...
https://github.com/Level/classic-level/actions/runs/6997605304/job/19034756860
but passed here: https://github.com/Level/classic-level/actions/runs/6997605338/job/19034756959
UPDATED: looks like there is a race condition. also failed here: https://github.com/Level/classic-level/actions/runs/6997605338/job/19034757374
Could be a race issue; can you check? I'll wait with the npm release.
For future reference (when the logs are deleted), the error is:
# check multithreading flag works as expected
not ok 5699 Error: Database is not open
---
operator: error
stack: |-
Error: Database is not open
at maybeOpened (/home/runner/work/classic-level/classic-level/node_modules/abstract-level/abstract-level.js:133:18)
at /home/runner/work/classic-level/classic-level/node_modules/abstract-level/abstract-level.js:160:13
at process.processTicksAndRejections (node:internal/process/task_queues:78:11)
...
not ok 5700 plan != count
---
operator: fail
expected: 9
actual: 1
stack: |-
Error: plan != count
at Test.assert [as _assert] (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:479:48)
at Test.fail (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:573:7)
at completeEnd (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:394:9)
at next (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:404:4)
at Test._end (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:424:2)
at Test.end (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:198:7)
at onError (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:133:10)
at process.processTicksAndRejections (node:internal/process/task_queues:96:5)
...
Could be a race issue; can you check? I'll wait with the npm release
Yep. Will take a look this evening
We have a use case for utilizing our level db from multiple worker threads. The base library did most of the heavy lifting and this PR allows the bindings to be used across workers.
Methodology
Context-awareness was preserved and so was threadsafe operation from the node.js perspective. The open and close functionality was all that needed to change and a single mutex protects the setup. A flag was added so the feature is non-breaking and only provides additional functionality.
The open_db function searches for a record in a
db_handles
map, and if it exists theopen_handle_count
is incremented for that database. This was used to allow for multiple databases to be accessed per process. The reverse happens when the destructor or the closed_db method is called. If the handle is the last to be closed then the db instance is deleted from the map to cleanup the nativeLOCK
.