FlowFuse / FlowFuse/nr-tables-nodes
Restructure code to make proper use of pools
- Dominant language
- JavaScript
- Stars
- 0
- Forks
- 0
- Avg merge
- 2d 19h
- Merged PRs (30d)
- 1
Description
### Description
Current implementation is (to be polite) not good.
**Big picture**: the node creates a DB pool asynchronously **per node** instance and streams results via a shared `cursor`/`getNextRows` state. There are several resource-management, concurrency and error-handling issues that can leak clients/pools or crash Node-RED.
**Findings (high priority)**
- There is no `node.on('close', ...)` to `end()` the Pool or close an active cursor. Deleting instances or full redeploys will leave pools/cursors open (resource leak).
- Asynchronous pool race: `pool` is created asynchronously via `ffAPI.getDatabases(...).then(...)`. If an input arrives before pool creation completes, code falls through to the "No database found" path or tries to use the placeholder object. Inputs should wait for pool readiness (or the node should reject gracefully).
- Cursor `callback` uses non-existent result in split mode the `pg-cursor` `callback` is used as `(err, rows, result)` and later references `result.command` / `result.rowCount`. `pg-cursor` callback signature is `(err, rows)`. Accessing `result` will be undefined and throw, likely crashing Node-RED.
- Shared per-node streaming state -> concurrency hazard: `cursor`, `getNextRows`, and `downstreamReady` are module-scope per node instance. They are shared by all concurrent messages. If two msgs cause overlapping queries the shared state leads to races and incorrect releases/closures.
- Cursor closing not awaited - `cursor.close()` is called without handling its async completion. This may leave pending operations.
- Global `getNextRows` nulling & closing ordering: `getNextRows = null` on done is correct, but there are paths where `getNextRows` remains and `cursor` is closed - subsequent calls may call `.read()` on a closed cursor.
**A better approach:**
- The pool should be created once and utelised by all nodes to avoid duplicated TCP connections, wasted resources, hitting DB connection limits, etc
- All async code should have error handling
- `error` events on the pool and other event emitters objects should be handled to avoid crashing NR
- Make per-request client/cursor lifetime local to that invocation; avoid sharing `cursor`/`getNextRows` across concurrent messages.
- Always release clients in finally blocks
- Ensure `cursor.close()` is awaited/handled (wrap in Promise if callback-style if reqd).
-`client.on('error',...)` listeners should be removed before releasing
- End the pool when the last node instance is removed.
- note: Also: always close `Cursor`s and release clients before calling `pool.end()` to avoid errors.
### Epic/Story
_No response_
Contributor guide
No contributing guide indexed for this repository
Research direction
No file or test is named in the issue. Locate the Node-RED node entry point and the code that creates the pool and reads the pg-cursor; trace node shutdown and overlapping input handling first. Done means request state is isolated, asynchronous errors and cleanup are handled, and pool shutdown waits until clients and cursors are closed.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, node.js, postgres
- Domain
- backend, databases
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 30/100