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.

Sign up for GitHub

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

pointer wrapper constructor #72

Merged
merged 1 commit into from
Apr 2, 2024
Merged

pointer wrapper constructor #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions generate-wrapper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ def visit_TypeDecl(self, node):
return

if isinstance(node.type, pycparser.c_ast.Struct):
self.cpp_result += f'exports.Set(Napi::String::New(env, "{name}"), duckdb_node::PointerHolder<{name}>::Init(env, "{name}")->Value());\n'
self.cpp_result += f'duckdb_node::PointerHolder<{name}>::Init(env, exports, "{name}");\n'
self.types_result += f'export class {name} {{}}\n'
self.c_type_to_ts_type[name] = name
self.c_type_to_ts_type[f'{name}*'] = name
Expand Down Expand Up @@ -263,11 +263,11 @@ def create_func_defs(filename):
types_out = open('lib/duckdb.d.ts', 'wb')

types_out.write('// placeholder interfaces for pointer types\n'.encode())
types_out.write('export interface pointer {}\n'.encode())
types_out.write('export interface uint64_pointer extends pointer {}\n'.encode())
types_out.write('export interface idx_pointer extends pointer {}\n'.encode())

types_out.write('// bindings-defined types\n'.encode())
types_out.write('export class pointer {}\n'.encode())
types_out.write('export class uint64_pointer {}\n'.encode())
types_out.write('export class out_string_wrapper {}\n'.encode())

types_out.write('// generated types and functions\n'.encode())
Expand Down
4 changes: 2 additions & 2 deletions lib/duckdb.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// placeholder interfaces for pointer types
export interface pointer {}
export interface uint64_pointer extends pointer {}
export interface idx_pointer extends pointer {}
// bindings-defined types
export class pointer {}
export class uint64_pointer {}
export class out_string_wrapper {}
// generated types and functions
export enum duckdb_type {
Expand Down
13 changes: 7 additions & 6 deletions src/duckdb_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,17 @@ class DuckDBNodeNative : public Napi::Addon<DuckDBNodeNative> {
DuckDBNodeNative(Napi::Env env, Napi::Object exports) {
RegisterGenerated(env, exports);

exports.Set(Napi::String::New(env, "sizeof_bool"), Napi::Number::New(env, sizeof(bool)));
duckdb_node::PointerHolder<void *>::Init(env, exports, "pointer");
duckdb_node::PointerHolder<uint64_t *>::Init(env, exports, "uint64_pointer");
// TODO: add idx_pointer?
duckdb_node::PointerHolder<duckdb_node::out_string_wrapper>::Init(env, exports, "out_string_wrapper");

exports.Set(Napi::String::New(env, "sizeof_bool"), Napi::Number::New(env, sizeof(bool)));
exports.Set(Napi::String::New(env, "copy_buffer"), Napi::Function::New<CopyBuffer>(env));

exports.Set(Napi::String::New(env, "copy_buffer_double"), Napi::Function::New<CopyBufferDouble>(env));

exports.Set(
Napi::String::New(env, "out_string_wrapper"),
duckdb_node::PointerHolder<duckdb_node::out_string_wrapper>::Init(env, "out_string_wrapper")->Value());
exports.Set(Napi::String::New(env, "out_get_string"), Napi::Function::New<OutGetString>(env));

// for binding; not exposed in TypeScript
exports.Set(Napi::String::New(env, "initialize"), Napi::Function::New<Initialize>(env));
}
};
Expand Down
104 changes: 34 additions & 70 deletions src/duckdb_node_generated.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,76 +93,40 @@ static void RegisterGenerated(Napi::Env env, Napi::Object exports) {
exports.DefineProperty(
Napi::PropertyDescriptor::Value("duckdb_statement_type", duckdb_statement_type_enum,
static_cast<napi_property_attributes>(napi_enumerable | napi_configurable)));
exports.Set(Napi::String::New(env, "duckdb_date"),
duckdb_node::PointerHolder<duckdb_date>::Init(env, "duckdb_date")->Value());
exports.Set(Napi::String::New(env, "duckdb_date_struct"),
duckdb_node::PointerHolder<duckdb_date_struct>::Init(env, "duckdb_date_struct")->Value());
exports.Set(Napi::String::New(env, "duckdb_time"),
duckdb_node::PointerHolder<duckdb_time>::Init(env, "duckdb_time")->Value());
exports.Set(Napi::String::New(env, "duckdb_time_struct"),
duckdb_node::PointerHolder<duckdb_time_struct>::Init(env, "duckdb_time_struct")->Value());
exports.Set(Napi::String::New(env, "duckdb_time_tz"),
duckdb_node::PointerHolder<duckdb_time_tz>::Init(env, "duckdb_time_tz")->Value());
exports.Set(Napi::String::New(env, "duckdb_time_tz_struct"),
duckdb_node::PointerHolder<duckdb_time_tz_struct>::Init(env, "duckdb_time_tz_struct")->Value());
exports.Set(Napi::String::New(env, "duckdb_timestamp"),
duckdb_node::PointerHolder<duckdb_timestamp>::Init(env, "duckdb_timestamp")->Value());
exports.Set(Napi::String::New(env, "duckdb_timestamp_struct"),
duckdb_node::PointerHolder<duckdb_timestamp_struct>::Init(env, "duckdb_timestamp_struct")->Value());
exports.Set(Napi::String::New(env, "duckdb_interval"),
duckdb_node::PointerHolder<duckdb_interval>::Init(env, "duckdb_interval")->Value());
exports.Set(Napi::String::New(env, "duckdb_hugeint"),
duckdb_node::PointerHolder<duckdb_hugeint>::Init(env, "duckdb_hugeint")->Value());
exports.Set(Napi::String::New(env, "duckdb_uhugeint"),
duckdb_node::PointerHolder<duckdb_uhugeint>::Init(env, "duckdb_uhugeint")->Value());
exports.Set(Napi::String::New(env, "duckdb_decimal"),
duckdb_node::PointerHolder<duckdb_decimal>::Init(env, "duckdb_decimal")->Value());
exports.Set(
Napi::String::New(env, "duckdb_query_progress_type"),
duckdb_node::PointerHolder<duckdb_query_progress_type>::Init(env, "duckdb_query_progress_type")->Value());
exports.Set(Napi::String::New(env, "duckdb_string_t"),
duckdb_node::PointerHolder<duckdb_string_t>::Init(env, "duckdb_string_t")->Value());
exports.Set(Napi::String::New(env, "duckdb_list_entry"),
duckdb_node::PointerHolder<duckdb_list_entry>::Init(env, "duckdb_list_entry")->Value());
exports.Set(Napi::String::New(env, "duckdb_column"),
duckdb_node::PointerHolder<duckdb_column>::Init(env, "duckdb_column")->Value());
exports.Set(Napi::String::New(env, "duckdb_vector"),
duckdb_node::PointerHolder<duckdb_vector>::Init(env, "duckdb_vector")->Value());
exports.Set(Napi::String::New(env, "duckdb_string"),
duckdb_node::PointerHolder<duckdb_string>::Init(env, "duckdb_string")->Value());
exports.Set(Napi::String::New(env, "duckdb_blob"),
duckdb_node::PointerHolder<duckdb_blob>::Init(env, "duckdb_blob")->Value());
exports.Set(Napi::String::New(env, "duckdb_result"),
duckdb_node::PointerHolder<duckdb_result>::Init(env, "duckdb_result")->Value());
exports.Set(Napi::String::New(env, "duckdb_database"),
duckdb_node::PointerHolder<duckdb_database>::Init(env, "duckdb_database")->Value());
exports.Set(Napi::String::New(env, "duckdb_connection"),
duckdb_node::PointerHolder<duckdb_connection>::Init(env, "duckdb_connection")->Value());
exports.Set(Napi::String::New(env, "duckdb_prepared_statement"),
duckdb_node::PointerHolder<duckdb_prepared_statement>::Init(env, "duckdb_prepared_statement")->Value());
exports.Set(
Napi::String::New(env, "duckdb_extracted_statements"),
duckdb_node::PointerHolder<duckdb_extracted_statements>::Init(env, "duckdb_extracted_statements")->Value());
exports.Set(Napi::String::New(env, "duckdb_pending_result"),
duckdb_node::PointerHolder<duckdb_pending_result>::Init(env, "duckdb_pending_result")->Value());
exports.Set(Napi::String::New(env, "duckdb_appender"),
duckdb_node::PointerHolder<duckdb_appender>::Init(env, "duckdb_appender")->Value());
exports.Set(Napi::String::New(env, "duckdb_config"),
duckdb_node::PointerHolder<duckdb_config>::Init(env, "duckdb_config")->Value());
exports.Set(Napi::String::New(env, "duckdb_logical_type"),
duckdb_node::PointerHolder<duckdb_logical_type>::Init(env, "duckdb_logical_type")->Value());
exports.Set(Napi::String::New(env, "duckdb_data_chunk"),
duckdb_node::PointerHolder<duckdb_data_chunk>::Init(env, "duckdb_data_chunk")->Value());
exports.Set(Napi::String::New(env, "duckdb_value"),
duckdb_node::PointerHolder<duckdb_value>::Init(env, "duckdb_value")->Value());
exports.Set(Napi::String::New(env, "duckdb_arrow"),
duckdb_node::PointerHolder<duckdb_arrow>::Init(env, "duckdb_arrow")->Value());
exports.Set(Napi::String::New(env, "duckdb_arrow_stream"),
duckdb_node::PointerHolder<duckdb_arrow_stream>::Init(env, "duckdb_arrow_stream")->Value());
exports.Set(Napi::String::New(env, "duckdb_arrow_schema"),
duckdb_node::PointerHolder<duckdb_arrow_schema>::Init(env, "duckdb_arrow_schema")->Value());
exports.Set(Napi::String::New(env, "duckdb_arrow_array"),
duckdb_node::PointerHolder<duckdb_arrow_array>::Init(env, "duckdb_arrow_array")->Value());
duckdb_node::PointerHolder<duckdb_date>::Init(env, exports, "duckdb_date");
duckdb_node::PointerHolder<duckdb_date_struct>::Init(env, exports, "duckdb_date_struct");
duckdb_node::PointerHolder<duckdb_time>::Init(env, exports, "duckdb_time");
duckdb_node::PointerHolder<duckdb_time_struct>::Init(env, exports, "duckdb_time_struct");
duckdb_node::PointerHolder<duckdb_time_tz>::Init(env, exports, "duckdb_time_tz");
duckdb_node::PointerHolder<duckdb_time_tz_struct>::Init(env, exports, "duckdb_time_tz_struct");
duckdb_node::PointerHolder<duckdb_timestamp>::Init(env, exports, "duckdb_timestamp");
duckdb_node::PointerHolder<duckdb_timestamp_struct>::Init(env, exports, "duckdb_timestamp_struct");
duckdb_node::PointerHolder<duckdb_interval>::Init(env, exports, "duckdb_interval");
duckdb_node::PointerHolder<duckdb_hugeint>::Init(env, exports, "duckdb_hugeint");
duckdb_node::PointerHolder<duckdb_uhugeint>::Init(env, exports, "duckdb_uhugeint");
duckdb_node::PointerHolder<duckdb_decimal>::Init(env, exports, "duckdb_decimal");
duckdb_node::PointerHolder<duckdb_query_progress_type>::Init(env, exports, "duckdb_query_progress_type");
duckdb_node::PointerHolder<duckdb_string_t>::Init(env, exports, "duckdb_string_t");
duckdb_node::PointerHolder<duckdb_list_entry>::Init(env, exports, "duckdb_list_entry");
duckdb_node::PointerHolder<duckdb_column>::Init(env, exports, "duckdb_column");
duckdb_node::PointerHolder<duckdb_vector>::Init(env, exports, "duckdb_vector");
duckdb_node::PointerHolder<duckdb_string>::Init(env, exports, "duckdb_string");
duckdb_node::PointerHolder<duckdb_blob>::Init(env, exports, "duckdb_blob");
duckdb_node::PointerHolder<duckdb_result>::Init(env, exports, "duckdb_result");
duckdb_node::PointerHolder<duckdb_database>::Init(env, exports, "duckdb_database");
duckdb_node::PointerHolder<duckdb_connection>::Init(env, exports, "duckdb_connection");
duckdb_node::PointerHolder<duckdb_prepared_statement>::Init(env, exports, "duckdb_prepared_statement");
duckdb_node::PointerHolder<duckdb_extracted_statements>::Init(env, exports, "duckdb_extracted_statements");
duckdb_node::PointerHolder<duckdb_pending_result>::Init(env, exports, "duckdb_pending_result");
duckdb_node::PointerHolder<duckdb_appender>::Init(env, exports, "duckdb_appender");
duckdb_node::PointerHolder<duckdb_config>::Init(env, exports, "duckdb_config");
duckdb_node::PointerHolder<duckdb_logical_type>::Init(env, exports, "duckdb_logical_type");
duckdb_node::PointerHolder<duckdb_data_chunk>::Init(env, exports, "duckdb_data_chunk");
duckdb_node::PointerHolder<duckdb_value>::Init(env, exports, "duckdb_value");
duckdb_node::PointerHolder<duckdb_arrow>::Init(env, exports, "duckdb_arrow");
duckdb_node::PointerHolder<duckdb_arrow_stream>::Init(env, exports, "duckdb_arrow_stream");
duckdb_node::PointerHolder<duckdb_arrow_schema>::Init(env, exports, "duckdb_arrow_schema");
duckdb_node::PointerHolder<duckdb_arrow_array>::Init(env, exports, "duckdb_arrow_array");
exports.Set(Napi::String::New(env, "duckdb_open"),
Napi::Function::New<duckdb_node::FunctionWrappers::AsyncFunctionWrapper2<
duckdb_state, const char *, duckdb_database *, "duckdb_open">>(env));
Expand Down
18 changes: 11 additions & 7 deletions src/value_conversion.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,14 @@ static Napi::Value GetValue(const Napi::CallbackInfo &info, size_t offset) {
template <class T>
class PointerHolder : public Napi::ObjectWrap<PointerHolder<T>> {
public:
static Napi::FunctionReference *Init(Napi::Env env, const char *name) {
static void Init(Napi::Env env, Napi::Object exports, const char *name) {
auto func = Napi::ObjectWrap<PointerHolder<T>>::DefineClass(env, name, {});
auto constructor = new Napi::FunctionReference();
*constructor = Napi::Persistent(func); // weird
env.SetInstanceData<Napi::FunctionReference>(constructor); // is this so this is eventually freed?
return constructor;
constructor = Napi::Persistent(func); // set initial reference count to 1
exports.Set(name, func);
// How does the following interact with the static storage of constructor?
// Does not doing this create problems for multiple instance of this add-on, in multiple worker threads?
// See: https://github.com/nodejs/node-addon-api/blob/main/doc/object_wrap.md#example
// env.SetInstanceData<Napi::FunctionReference>(&constructor); // is this so this is eventually freed?
Copy link
Contributor Author

@jraymakers jraymakers Mar 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that storing this function reference in a static might be a problem if multiple instance of this add-on are created in the same Node process, in different environments (e.g. worker threads). (More details here.) Not sure how likely or useful that is, but I'd rather not create a potential problem if I don't have to.

But we can't store these constructors directly using SetInstanceData, because there is only one "data" slot per add-on instance, and we have multiple PointerHolders.

Ideas for how to do this better:

  • Store a map from name to constructor in the instance data. This means we'd need to keep the name around in the PointerHolder (so we can look up the right constructor), but putting the name in a static seems fine. This is a bit more work to set up and a bit more overhead at runtime, but these are likely insignificant.
  • Alternatively, since we're already storing the constructor in the exports, it seems like it should be possible to look it up using the environment (e.g. using Globals). I haven't quite figured that out, but I probably just haven't learned the right way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I just learned (by reading more docs) that the Napi::Addon uses Napi::Env::SetInstanceData internally, so it shouldn't be used to store other data:

Note: Napi::Addon uses Napi::Env::SetInstanceData() internally. This means that the add-on should only use Napi::Env::GetInstanceData explicitly to retrieve the instance of the Napi::Addon class. Variables whose scope would otherwise be global should be stored as instance variables in the Napi::Addon class.

The recommendation is to store stuff directly on your Addon instance object, and use GetInstanceData to get the Addon instance:

void ExampleBinding(const Napi::CallbackInfo& info) {
  ExampleAddon* addon = info.Env().GetInstanceData<ExampleAddon>();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's where node-addon-api puts the addon instance into the instance data: https://github.com/nodejs/node-addon-api/blob/cfcd2cf61d8d939bc2a2744af2ee72f07e4119c1/napi-inl.h#L6518

}

PointerHolder(const Napi::CallbackInfo &info) : Napi::ObjectWrap<PointerHolder<T>>(info) {
Expand All @@ -45,8 +47,7 @@ class PointerHolder : public Napi::ObjectWrap<PointerHolder<T>> {
}

static Napi::Value NewAndSet(Napi::Env &env, T val) {
// TODO use actual constructor here ?
auto res = PointerHolder<T>::DefineClass(env, "duckdb_pointer_holder", {}, nullptr).New({});
auto res = constructor.New({});
Napi::ObjectWrap<PointerHolder<T>>::Unwrap(res)->Set(val);
return res;
}
Expand All @@ -59,9 +60,12 @@ class PointerHolder : public Napi::ObjectWrap<PointerHolder<T>> {
}

private:
static Napi::FunctionReference constructor;
std::unique_ptr<data_t[]> ptr;
};

template<class T> Napi::FunctionReference PointerHolder<T>::constructor;

struct out_string_wrapper {
const char *ptr;
};
Expand Down
Loading