-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(NODE-5409)!: allow socks to be installed optionally #3782
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,11 @@ | ||
import type { Socket, SocketConnectOpts } from 'net'; | ||
import * as net from 'net'; | ||
import { SocksClient } from 'socks'; | ||
import type { ConnectionOptions as TLSConnectionOpts, TLSSocket } from 'tls'; | ||
import * as tls from 'tls'; | ||
|
||
import type { Document } from '../bson'; | ||
import { LEGACY_HELLO_COMMAND } from '../constants'; | ||
import { getSocks, type SocksLib } from '../deps'; | ||
import { | ||
MongoCompatibilityError, | ||
MongoError, | ||
|
@@ -419,6 +419,18 @@ function makeConnection(options: MakeConnectionOptions, _callback: Callback<Stre | |
} | ||
} | ||
|
||
let socks: SocksLib | null = null; | ||
function loadSocks() { | ||
if (socks == null) { | ||
const socksImport = getSocks(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment as in stateMachine.js. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto response |
||
if ('kModuleError' in socksImport) { | ||
throw socksImport.kModuleError; | ||
} | ||
socks = socksImport; | ||
} | ||
return socks; | ||
} | ||
|
||
function makeSocks5Connection(options: MakeConnectionOptions, callback: Callback<Stream>) { | ||
const hostAddress = HostAddress.fromHostPort( | ||
options.proxyHost ?? '', // proxyHost is guaranteed to set here | ||
|
@@ -434,7 +446,7 @@ function makeSocks5Connection(options: MakeConnectionOptions, callback: Callback | |
proxyHost: undefined | ||
}, | ||
(err, rawSocket) => { | ||
if (err) { | ||
if (err || !rawSocket) { | ||
return callback(err); | ||
} | ||
|
||
|
@@ -445,8 +457,14 @@ function makeSocks5Connection(options: MakeConnectionOptions, callback: Callback | |
); | ||
} | ||
|
||
try { | ||
socks ??= loadSocks(); | ||
} catch (error) { | ||
return callback(error); | ||
} | ||
|
||
// Then, establish the Socks5 proxy connection: | ||
SocksClient.createConnection({ | ||
socks.SocksClient.createConnection({ | ||
existing_socket: rawSocket, | ||
timeout: options.connectTimeoutMS, | ||
command: 'connect', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
makeModuleError
returns a proxy that throws whenever a key that isn'tkModuleError
is accessed. So I'd expect we would just do the following (where it's used):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm alinging with the approach taken for kerberos, zstd, snappy. The goal is to not invoke the import operation more than once, determined by the nullishness of the
socks
variable.