fix: remove constructor.Reset() from S7Client/S7Server destructors - #107
Open
JordiB-Inserma wants to merge 2 commits into
Open
fix: remove constructor.Reset() from S7Client/S7Server destructors#107JordiB-Inserma wants to merge 2 commits into
JordiB-Inserma wants to merge 2 commits into
Conversation
S7Client::~S7Client() and S7Server::~S7Server() were calling constructor.Reset() on a static Nan::Persistent<FunctionTemplate>. This releases the persistent handle to the FunctionTemplate that V8 uses to create new instances of the object. Since constructor is a static class member shared across ALL instances, destroying ANY single instance invalidates the constructor template for every existing and future instance. When V8's GC collects the invalidated template and a new instance is created, S7Client::New tries to resolve the constructor via FunctionTemplate::GetFunction() which returns a stale/collected handle, causing a segfault. This crash is non-deterministic and depends on V8 GC timing. On Linux, the GC is more aggressive, making the crash frequent in applications with repeated create/destroy cycles (e.g. SCADA/HMI reconnection loops). The fix is simply removing constructor.Reset() from both destructors. The static FunctionTemplate should live for the lifetime of the module, not per-instance. References: - davenardella/snap7#29 (upstream pthread fix, prerequisite for full Linux stability) - davenardella/snap7#19 (server destructor race)
Add AGENTS.md, .opencode/, .claude/, .cursorrules, and .github/copilot-instructions.md to both .gitignore and .npmignore to keep AI assistant configuration files out of version control and npm package distribution.
4 tasks
JordiB-Inserma
added a commit
to JordiB-Inserma/FUXA
that referenced
this pull request
Jul 28, 2026
Move datatypes initialization from create() into the S7client constructor, using the real s7client instance that is retained for the connection lifetime instead of creating a throwaway S7Client solely to read S7WL* constants. The throwaway instance was garbage collected immediately, and its destructor called constructor.Reset() on the shared Nan::Persistent<FunctionTemplate> in node-snap7, corrupting the prototype for all subsequent instances — causing segfaults on Linux (mathiask88/node-snap7#107). The guard if (!datatypes) ensures initialization runs only once. The S7WL* constants are prototype properties (not statics on the constructor), so an instance is required to read them — but the real instance is retained in the closure, avoiding the destructor path entirely. Supersedes d876199 (add missing new keyword) which corrected syntax but did not address the lifecycle issue.
unocelli
pushed a commit
to frangoteam/FUXA
that referenced
this pull request
Jul 30, 2026
…2437) * fix(s7): add missing 'new' keyword in S7Client instantiation The datatypes initialization in create() called snap7.S7Client() without the 'new' keyword, creating a throwaway instance that would be garbage-collected immediately. On Linux, the GC triggers the destructor which calls constructor.Reset() on the static Nan::Persistent<FunctionTemplate>, corrupting the shared prototype for all subsequent instances — causing a segfault. This is the application-side counterpart of the fix applied in mathiask88/node-snap7#107 (constructor.Reset() removal in ~S7Client/~S7Server destructors). Upstream references: - mathiask88/node-snap7#107: remove constructor.Reset() in destructors - davenardella/snap7#29: fix POSIX thread joinable lifecycle * fix(s7): properly reject promises on read/write errors _readVars, _writeDB, and _writeVars had a critical bug where callback errors were handled with 'return _getErr(err)' instead of 'return reject(_getErr(err))'. Since _getErr only formats the error string, the Promise was left in a permanently pending state — the resolve/reject callbacks were never called. This caused: - working flag stuck at true, blocking all subsequent operations - overloading counter to accumulate without reset - forced device reconnection (in _checkWorking) to recover - amplified create/destroy cycles that trigger constructor.Reset() segfaults on Linux (see mathiask88/node-snap7#107) Applied to three functions: - _readVars: ReadMultiVars callback - _writeDB: DBWrite callback - _writeVars: WriteMultiVars callback * fix(s7): remove forced disconnect on polling overload When _checkWorking detected repeated overloads (working=true while a new poll arrived), it called s7client.Disconnect() after 3 consecutive overloads. This forced disconnection triggered the device's cleanup/reconnect cycle, which creates and destroys S7Client instances — amplifying the constructor.Reset() segfault on Linux (mathiask88/node-snap7#107). The overload is a normal condition when polling intervals are shorter than PLC response times. Instead of disconnecting, the poll is now simply skipped (returns false), allowing the current operation to complete naturally. The overloading counter still increments and logs a warning for diagnostics. * fix(s7): lazy init datatypes from real S7Client instance Move datatypes initialization from create() into the S7client constructor, using the real s7client instance that is retained for the connection lifetime instead of creating a throwaway S7Client solely to read S7WL* constants. The throwaway instance was garbage collected immediately, and its destructor called constructor.Reset() on the shared Nan::Persistent<FunctionTemplate> in node-snap7, corrupting the prototype for all subsequent instances — causing segfaults on Linux (mathiask88/node-snap7#107). The guard if (!datatypes) ensures initialization runs only once. The S7WL* constants are prototype properties (not statics on the constructor), so an instance is required to read them — but the real instance is retained in the closure, avoiding the destructor path entirely. Supersedes d876199 (add missing new keyword) which corrected syntax but did not address the lifecycle issue.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
S7Client and S7Server destructors were calling
constructor.Reset()on a staticNan::Persistent<FunctionTemplate>, which destroys the V8 constructor template shared across all instances. This causes segfaults on Linux when applications create and destroyS7Clientinstances repeatedly (e.g. SCADA/HMI reconnection loops like FUXA).Root cause
constructoris a static class member initialized once during module load (Init). It stores theFunctionTemplatethat V8 uses to create new instances. Both~S7Client()and~S7Server()were callingconstructor.Reset(), which releases the persistent handle to the static FunctionTemplate.Since this is a static resource shared across ALL instances:
new snap7.S7Client()— works, FunctionTemplate aliveconstructor.Reset()destroys the static templatenew snap7.S7Client()→ V8's internal constructor resolution fails → SEGFAULTThe crash is non-deterministic and depends on V8 GC timing. On Linux, the GC is more aggressive, making this crash frequent in production SCADA systems.
Fix
Remove
constructor.Reset()from both~S7Client()(node_snap7_client.cpp:722) and~S7Server()(node_snap7_server.cpp:682). The static FunctionTemplate should live for the lifetime of the module, not per-instance.Changes
src/node_snap7_client.cppconstructor.Reset()from~S7Client()src/node_snap7_server.cppconstructor.Reset()from~S7Server()Stack trace before fix (Linux)
Related