Node.js connection idle sockets timeout rework, logger improvements #242
Conversation
| @@ -26,12 +26,12 @@ jobs: | |||
| strategy: | |||
| fail-fast: true | |||
| matrix: | |||
| node: [16, 18, 20] | |||
| node: [18, 20, 21] | |||
There was a problem hiding this comment.
21 is not LTS and might be unstable
There was a problem hiding this comment.
It's worth keeping if it does not bring any flakiness to the CI run. If it starts to do so, we will remove it.
| @@ -51,6 +51,8 @@ export type ConnPingResult = | |||
| } | |||
| | { success: false; error: Error } | |||
|
|
|||
| export type ConnOperation = 'Ping' | 'Query' | 'Insert' | 'Exec' | |||
There was a problem hiding this comment.
nit: I'd rather use the fullname ConnectorOperation or just Operation to avoid any abbreviations
There was a problem hiding this comment.
I will rename all of these types in 1.0.0, then. They are all prefixed with just Conn.
| this.logger.trace(params) | ||
| this.logger.trace({ | ||
| ...params, | ||
| module: params.module ?? this.module, |
There was a problem hiding this comment.
enhancement request: context-tree like in log4j
| server.close() | ||
| }) | ||
|
|
||
| // ping first, then 2 operations in all possible combinations - repeat every combination several times |
| expect().nothing() | ||
| }) | ||
|
|
||
| it('should not throw unhandled errors with Select', async () => { |
There was a problem hiding this comment.
I don't think we test this. The test verifies select throws Timeout error. exception. How does just runner handles uncaught exceptions / promise rejections?
https://developer.mozilla.org/en-US/docs/Web/API/Window/unhandledrejection_event
https://nodejs.org/api/process.html#event-unhandledrejection
There was a problem hiding this comment.
The test will still fail if something is thrown after the timeout error and it is unhandled (like something thrown from the Node's HTTP internals after we get the timeout error). This was the main problem with Ping, actually. That's also why there is a test of Ping + a few other operations in sequence.
Summary
Initially, this PR was planned for 1.0.0, but it will be safer to release this as 0.3.0-beta.1 in advance to test this particular changeset.
Resolves #237
Resolves #196 (it's not that KeepAlive was degrading performance; it was that KeepAlive=false was still using the entire KeepAlive mechanism).
Changes (some are breaking):
Connectionheader value considering KeepAlive settings. If KeepAlive is disabled, its value is now forced to "close".keep_alive.retry_on_expired_socketandkeep_alive.socket_ttlconfiguration parameters are removed. Instead, the newkeep_alive.idle_socket_ttlparameter should be used. The default value forkeep_alive.idle_socket_ttlis 2500.max_open_connectionsconfiguration parameter is now 10 by default, as we should not rely on the KeepAlive agent's defaults.request_timeoutconfiguration value (now it is correctly set to 30s).OFFin the client configuration (it is off by default)); all client methods except ping will log an error on failure now, and ping will log a warning (cause the error is returned as part of its result).toURLSearchParamssince query_id has not been optional within the connection for a long time and is present for any request that uses it.A changelog entry will be added if we decide to proceed with this.
Checklist