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

fix: handle timeouts during tls negotiation for strict encryption #1564

Merged
merged 10 commits into from Sep 17, 2023

Conversation

MichaelSun90
Copy link
Contributor

Network socket not correctly cleaned up when connection timeout fires during TDS8.0 TLS negotiation #1558

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #1564 (33ab74b) into master (4f3e210) will increase coverage by 0.21%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master    #1564      +/-   ##
==========================================
+ Coverage   80.45%   80.67%   +0.21%     
==========================================
  Files          92       92              
  Lines        4692     4709      +17     
  Branches      871      871              
==========================================
+ Hits         3775     3799      +24     
+ Misses        644      633      -11     
- Partials      273      277       +4     
Files Changed Coverage Δ
src/connection.ts 65.97% <95.65%> (+1.23%) ⬆️

@MichaelSun90
Copy link
Contributor Author

@arthurschreiber remove the TDS 7_1 checks against SQL server 2022 since 7_1 suppose not work with SQL server 2022 and it did failing the encrypt tests. We did no run into this because the added encrypt tests are not actually running due to a error within our test. If we still want to keep the 7_1 test run against SQL server 2022 for certain reason, we can also add some logic to skip the tests like that we did in 'should reset Connection'.

src/connection.ts Outdated Show resolved Hide resolved
src/connection.ts Outdated Show resolved Hide resolved
src/connection.ts Outdated Show resolved Hide resolved
@arthurschreiber arthurschreiber changed the title chore: handle timeout for tlssocket fix: handle timeout for tlssocket Aug 2, 2023
@MichaelSun90
Copy link
Contributor Author

Figured out, that signal.reason can be undefined which causing an additional error if we do tostring and compare it with the error that has been caught see if it include the abort title. However, just err === signal.reason still does not handles the case that :
signal.reason :

Error: AbortError
    at AbortController.abort (C:\Users\msun\Documents\GitHub\tedious\node_modules\node-abort-controller\index.js:56:31)
    at Timeout._onTimeout (C:\Users\msun\Documents\GitHub\tedious\src\/connection.ts:2116:18)
    at listOnTimeout (node:internal/timers:569:17)
    at processTimers (node:internal/timers:512:7)

err is:

AbortError: The operation was aborted
    at EventEmitter.onAbort (C:\Users\msun\Documents\GitHub\tedious\src\/connector.ts:108:18)
    at EventEmitter.emit (node:events:514:28)
    at AbortSignal.dispatchEvent (C:\Users\msun\Documents\GitHub\tedious\node_modules\node-abort-controller\index.js:28:23)
    at AbortController.abort (C:\Users\msun\Documents\GitHub\tedious\node_modules\node-abort-controller\index.js:58:17)
    at Timeout._onTimeout (C:\Users\msun\Documents\GitHub\tedious\src\/connection.ts:2116:18)
    at listOnTimeout (node:internal/timers:569:17)
    at processTimers (node:internal/timers:512:7) {
  code: 'ABORT_ERR'
}
if (err === signal.reason || err.name === 'AbortError'){
    return;
}
if(signal.reason && signal.reason.tostring().include('AbortError')) {
     return;
}

both of these will handle the errors correctly, but not exactly a clean solution in my opinion.
Not sure if we should go with either one or @arthurschreiber you have suggestions for handling this.

@arthurschreiber arthurschreiber changed the title fix: handle timeout for tlssocket fix: handle timeouts during tls negatiation for strict encryption Sep 17, 2023
@arthurschreiber
Copy link
Collaborator

@MichaelSun90 I changed the code to check for signal.aborted instead, and I think this now has the correct behaviour.

If the signal (which is coming from the connectTimeout) was aborted, that means that the connect timeout has fired and we need to ignore that error (because a different error will be generated by the connect timeout code.

Anyway, hopefully this can be cleaned up via the changes in #1562.

@arthurschreiber arthurschreiber changed the title fix: handle timeouts during tls negatiation for strict encryption fix: handle timeouts during tls negotiation for strict encryption Sep 17, 2023
@arthurschreiber arthurschreiber merged commit 8c7e440 into master Sep 17, 2023
27 checks passed
@arthurschreiber arthurschreiber deleted the michael-TLStimeout branch September 17, 2023 11:34
@github-actions
Copy link

🎉 This PR is included in version 16.4.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MichaelSun90
Copy link
Contributor Author

@MichaelSun90 I changed the code to check for signal.aborted instead, and I think this now has the correct behaviour.

If the signal (which is coming from the connectTimeout) was aborted, that means that the connect timeout has fired and we need to ignore that error (because a different error will be generated by the connect timeout code.

Anyway, hopefully this can be cleaned up via the changes in #1562.

Thanks for help on this change, and release it ! 🙇‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants