feat: get printers shared via cups #49

Good work on implementing the CUPS printer discovery feature. The C++ native module compiles cleanly, passes both clang-tidy and clang-format checks, and the overall architecture (using Napi::AsyncWorker for blocking I/O) is correct.

Here are my findings organized by severity:

🔴 Critical Issues

1. Use-after-free race condition in cups-get-dests.cpp

In CupsGetDestsWorker, the raw http_t* pointer is stored from External::Data(). If the CupsConnection JS object is garbage collected while the worker is executing Execute(), the finalizer will call httpClose() on the underlying pointer, but the worker thread will still be using it — leading to a use-after-free.

The External is only kept alive by the JS-side CupsConnection.data reference, but the native worker doesn't hold a strong reference to it during execution.

Fix: Store a Napi::ObjectReference (or Napi::Persistent) to the JS object in the worker to keep it alive during execution:

Napi::ObjectReference connectionRef;
// In constructor:
this->connectionRef.Reset(connection.As<Napi::Object>(), 1);
// In destructor:
this->connectionRef.Reset();

🟡 Medium Issues

2. No explicit resource cleanup for CupsConnection

The CupsConnection class has no destroy() or close() method. The underlying http_t* is only closed when the External finalizer runs, which is non-deterministic. In long-running servers, this could lead to leaked HTTP connections.

Suggestion: Add a close() method:

close(): void {
  // The External finalizer will handle httpClose when GC'd,
  // but we can clear our reference here.
  this.data = undefined as unknown as CupsConnectionData;
}

3. Empty stub methods in CupsPrinter

getInfo() and sendJob() are empty bodies. These should either be implemented or marked with // TODO: comments so consumers know they're not yet available.

4. Unused exported type CupsPrinterInfoData

This branded type is exported but never used anywhere. It appears to be a placeholder for future getInfo() implementation. Either use it or remove it to avoid confusion.

🟢 Minor / Style

5. CupsPrinter creates a circular reference

CupsPrinter holds a reference to CupsConnection, which in turn would hold references to CupsPrinter (if added to a list). This is handled by the JS GC, but it means CupsConnection cannot be collected while any CupsPrinter references exist. Consider whether CupsPrinter needs to hold the connection reference at all, or if it could be accessed lazily.

6. Buffer size constants in http-connect-uri.cpp

The local variables schemeLen, usernameLen, etc. are declared const auto but are only used in one place. Consider simplifying:

auto schemeBuf = std::array<char, HTTP_MAX_URI>();

7. Missing C++23 requirement in docs

The meson.build upgrades to C++23 for std::views::enumerate, but the README and AGENTS.md don't mention a minimum C++23 compiler requirement. Consider adding this to the prerequisites.

✅ Positive Notes

Clean separation of concerns: http-connect-uri.cpp and cups-get-dests.cpp each handle a single responsibility
Proper use of Napi::AsyncWorker for blocking libcups calls
Correct memory management with External finalizers (httpClose and cupsFreeDests)
Good documentation comments pointing to libcups v2/v3 docs
Meson configuration is clean with is_system: true for system headers
The TypeScript branding pattern (unknown & { __brand: "..." }) provides nominal type safety

## Review Summary Good work on implementing the CUPS printer discovery feature. The C++ native module compiles cleanly, passes both clang-tidy and clang-format checks, and the overall architecture (using `Napi::AsyncWorker` for blocking I/O) is correct. Here are my findings organized by severity: ### 🔴 Critical Issues **1. Use-after-free race condition in `cups-get-dests.cpp`** In `CupsGetDestsWorker`, the raw `http_t*` pointer is stored from `External::Data()`. If the `CupsConnection` JS object is garbage collected while the worker is executing `Execute()`, the finalizer will call `httpClose()` on the underlying pointer, but the worker thread will still be using it — leading to a use-after-free. The `External` is only kept alive by the JS-side `CupsConnection.data` reference, but the native worker doesn't hold a strong reference to it during execution. **Fix:** Store a `Napi::ObjectReference` (or `Napi::Persistent`) to the JS object in the worker to keep it alive during execution: ```cpp Napi::ObjectReference connectionRef; // In constructor: this->connectionRef.Reset(connection.As<Napi::Object>(), 1); // In destructor: this->connectionRef.Reset(); ``` ### 🟡 Medium Issues **2. No explicit resource cleanup for `CupsConnection`** The `CupsConnection` class has no `destroy()` or `close()` method. The underlying `http_t*` is only closed when the `External` finalizer runs, which is non-deterministic. In long-running servers, this could lead to leaked HTTP connections. **Suggestion:** Add a `close()` method: ```typescript close(): void { // The External finalizer will handle httpClose when GC'd, // but we can clear our reference here. this.data = undefined as unknown as CupsConnectionData; } ``` **3. Empty stub methods in `CupsPrinter`** `getInfo()` and `sendJob()` are empty bodies. These should either be implemented or marked with `// TODO:` comments so consumers know they're not yet available. **4. Unused exported type `CupsPrinterInfoData`** This branded type is exported but never used anywhere. It appears to be a placeholder for future `getInfo()` implementation. Either use it or remove it to avoid confusion. ### 🟢 Minor / Style **5. `CupsPrinter` creates a circular reference** `CupsPrinter` holds a reference to `CupsConnection`, which in turn would hold references to `CupsPrinter` (if added to a list). This is handled by the JS GC, but it means `CupsConnection` cannot be collected while any `CupsPrinter` references exist. Consider whether `CupsPrinter` needs to hold the connection reference at all, or if it could be accessed lazily. **6. Buffer size constants in `http-connect-uri.cpp`** The local variables `schemeLen`, `usernameLen`, etc. are declared `const auto` but are only used in one place. Consider simplifying: ```cpp auto schemeBuf = std::array<char, HTTP_MAX_URI>(); ``` **7. Missing C++23 requirement in docs** The meson.build upgrades to C++23 for `std::views::enumerate`, but the README and AGENTS.md don't mention a minimum C++23 compiler requirement. Consider adding this to the prerequisites. ### ✅ Positive Notes - Clean separation of concerns: `http-connect-uri.cpp` and `cups-get-dests.cpp` each handle a single responsibility - Proper use of `Napi::AsyncWorker` for blocking libcups calls - Correct memory management with `External` finalizers (`httpClose` and `cupsFreeDests`) - Good documentation comments pointing to libcups v2/v3 docs - Meson configuration is clean with `is_system: true` for system headers - The TypeScript branding pattern (`unknown & { __brand: "..." }`) provides nominal type safety

cups/src/cups-get-dests.cpp Outdated

					
				@ -0,0 +10,4 @@

				  Napi::Promise::Deferred deferred;

				  http_t* connection;

				  std::vector<cups_dest_t> dests;

Use-after-free: raw http_t* stored from External::Data() without keeping JS object alive during worker execution. If CupsConnection is GC'd mid-execution, httpClose() frees the pointer while worker thread still uses it.