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

Prevent node-red crash on exit #92

Merged
merged 2 commits into from
Feb 17, 2024
Merged

Conversation

Steve-Mcl
Copy link
Collaborator

handle the fact pool.close is now a promise.

This fix is only to contain the issue raised in #91

A future change is necessary to make the node.on('close', callback should be async and the .disconnect through to connectionCleanup functions should await so that we correctly inform node-red that it is OK to close

        node.on('close', function () {
            mssqlCN.disconnect(node.id);
        });

NOTE: this is untested - will request review before merge.

@Steve-Mcl Steve-Mcl requested a review from bestlong December 21, 2023 11:44
@bombjackit
Copy link

bombjackit commented Feb 14, 2024

@bestlong and @Steve-Mcl in my environment this commit works as expected.

@Steve-Mcl
Copy link
Collaborator Author

@bestlong are you able to review and merge?

@bombjackit
Copy link

@Steve-Mcl could you change dependecies of this node to use at minimun version 10.0.2 of node mssql ? In this version there are another node-red related bug fix (see tediousjs/node-mssql#1592).

Thanks in adnvance

@Steve-Mcl
Copy link
Collaborator Author

@Steve-Mcl could you change dependecies of this node to use at minimun version 10.0.2 of node mssql ? In this version there are another node-red related bug fix (see tediousjs/node-mssql#1592).

Thanks in adnvance

@bombjackit dependency ^10.0.2 satisfies constraint ^10.0.0. npm install should pull the newer dependency.

You can check this by running npm list mssql in the .node-red directory to confirm.

@bestlong bestlong merged commit fcb66c7 into master Feb 17, 2024
@Steve-Mcl Steve-Mcl deleted the temp-fix-for-exit-crashing-nr branch February 17, 2024 11:15
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.

3 participants