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

Support backing up to an instance of sqlite3.Database #1649

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

SmartArray
Copy link

This PR introduces the ability to use an instance of sqlite3.Database to perform backups using the Online Backup API.

This PR is a work in progress until yarn test runs through successfully.

@SmartArray
Copy link
Author

SmartArray commented Oct 25, 2022

Help needed

The call to the backup API is causing a segfault, currently.

I suppose this line is causing the issue: https://github.com/SmartArray/node-sqlite3/blob/401937943af31cd6e7844deb124e315df4c58400/src/backup.cc#L161

I could not figure out why the destructor of the Backup instance causes this bad memory access (a double free)?

I would love some assistance in this issue! Unfortunately I have no experience using the N-API.

Here is the backtrace I gathered with LLDB:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbeaddccdaa08)
  * frame #0: 0x0000000100078e20 node`napi_delete_reference + 52
    frame #1: 0x0000000107cef270 node_sqlite3.node`Napi::Reference<Napi::Object>::~Reference() + 76
    frame #2: 0x0000000107ceba18 node_sqlite3.node`Napi::ObjectWrap<node_sqlite3::Backup>::~ObjectWrap() + 252
    frame #3: 0x0000000107cfba08 node_sqlite3.node`node_sqlite3::Backup::~Backup() + 196
    frame #4: 0x0000000107ceeb28 node_sqlite3.node`node_sqlite3::Backup::~Backup() + 28
    frame #5: 0x0000000107ceeb54 node_sqlite3.node`node_sqlite3::Backup::~Backup() + 28
    frame #6: 0x0000000107cf53dc node_sqlite3.node`Napi::ObjectWrap<node_sqlite3::Backup>::ConstructorCallbackWrapper(napi_env__*, napi_callback_info__*)::'lambda'()::operator()() const + 212
    frame #7: 0x0000000107cf52e8 node_sqlite3.node`napi_value__* Napi::details::WrapCallback<Napi::ObjectWrap<node_sqlite3::Backup>::ConstructorCallbackWrapper(napi_env__*, napi_callback_info__*)::'lambda'()>(Napi::ObjectWrap<node_sqlite3::Backup>::ConstructorCallbackWrapper(napi_env__*, napi_callback_info__*)::'lambda'()) + 32
    frame #8: 0x0000000107cf3eec node_sqlite3.node`Napi::ObjectWrap<node_sqlite3::Backup>::ConstructorCallbackWrapper(napi_env__*, napi_callback_info__*) + 148
    frame #9: 0x000000010007b36c node`v8impl::(anonymous namespace)::FunctionCallbackWrapper::Invoke(v8::FunctionCallbackInfo<v8::Value> const&) + 108
    frame #10: 0x0000000100270a38 node`v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) + 276
    frame #11: 0x000000010027026c node`v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<true>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) + 504
    frame #12: 0x000000010026fd9c node`v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) + 196
    frame #13: 0x00000001009b088c node`Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit + 108
    frame #14: 0x0000000100941610 node`Builtins_JSBuiltinsConstructStub + 368
    frame #15: 0x000000011010e934
    frame #16: 0x00000001009443d8 node`Builtins_InterpreterEntryTrampoline + 248
    frame #17: 0x00000001009421cc node`Builtins_JSEntryTrampoline + 172
    frame #18: 0x0000000100941e64 node`Builtins_JSEntry + 164
    frame #19: 0x0000000100313a24 node`v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) + 2388
    frame #20: 0x00000001003130b8 node`v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) + 200
    frame #21: 0x0000000100220548 node`v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) + 520
    frame #22: 0x0000000100076924 node`napi_call_function + 168
    frame #23: 0x0000000107cefecc node_sqlite3.node`Napi::Function::Call(napi_value__*, unsigned long, napi_value__* const*) const + 64
    frame #24: 0x0000000107ce9c10 node_sqlite3.node`Napi::Function::Call(napi_value__*, std::__1::vector<napi_value__*, std::__1::allocator<napi_value__*> > const&) const + 80
    frame #25: 0x0000000107d00618 node_sqlite3.node`node_sqlite3::Database::Work_AfterOpen(napi_env__*, napi_status, void*) + 1468
    frame #26: 0x000000010008af98 node`(anonymous namespace)::uvimpl::Work::AfterThreadPoolWork(int) + 136
    frame #27: 0x0000000100922764 node`uv__work_done + 192
    frame #28: 0x0000000100925f00 node`uv__async_io + 320
    frame #29: 0x0000000100937c78 node`uv__io_poll + 1036
    frame #30: 0x0000000100926390 node`uv_run + 380
    frame #31: 0x0000000100002ccc node`node::SpinEventLoop(node::Environment*) + 244
    frame #32: 0x00000001000ed6e0 node`node::NodeMainInstance::Run(int*, node::Environment*) + 120
    frame #33: 0x00000001000ed3ac node`node::NodeMainInstance::Run(node::EnvSerializeInfo const*) + 120
    frame #34: 0x00000001000872e0 node`node::Start(int, char**) + 184
    frame #35: 0x000000010499d08c dyld`start + 520

@SmartArray
Copy link
Author

Fixed all memory issues, tests and added typescript declarations.
Integrated into production environment of a project. Seems to work well.

@SmartArray SmartArray marked this pull request as ready for review October 25, 2022 09:10
if (info[1].IsObject()) {
// A database instance was passed instead of a filename
otherDb = Napi::ObjectWrap<Database>::Unwrap(info[1].As<Napi::Object>());
otherDb->Ref();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if the Ref() call is needed here. I have applied this from @paulfitz.
I assume it is needed because it prevents the otherDb instance to be freed while a backup is in progress, which should usually not happen unless the user closes the db during a backup process.

Please someone double check.

@SmartArray SmartArray changed the title [WIP] Support backing up to an instance of sqlite3.Database Support backing up to an instance of sqlite3.Database Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants