Skip to content

Commit 9b85557

Browse files
[PECO-745] Default request timeout (#148)
* [PECO-745] Default request timeout Signed-off-by: Levko Kravets <levko.ne@gmail.com> * Expose socket timeout option; add tests Signed-off-by: Levko Kravets <levko.ne@gmail.com> * Fix patch Signed-off-by: Levko Kravets <levko.ne@gmail.com> --------- Signed-off-by: Levko Kravets <levko.ne@gmail.com>
1 parent 39ba279 commit 9b85557

File tree

7 files changed

+78
-5
lines changed

7 files changed

+78
-5
lines changed

lib/connection/connections/HttpConnection.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
import thrift from 'thrift';
22
import https from 'https';
3-
43
import http, { IncomingMessage } from 'http';
4+
55
import IThriftConnection from '../contracts/IThriftConnection';
66
import IConnectionProvider from '../contracts/IConnectionProvider';
77
import IConnectionOptions, { Options } from '../contracts/IConnectionOptions';
88
import IAuthentication from '../contracts/IAuthentication';
99
import HttpTransport from '../transports/HttpTransport';
10+
import globalConfig from '../../globalConfig';
1011

1112
type NodeOptions = {
1213
ca?: Buffer | string;
@@ -25,6 +26,7 @@ export default class HttpConnection implements IConnectionProvider, IThriftConne
2526
keepAlive: true,
2627
maxSockets: 5,
2728
keepAliveMsecs: 10000,
29+
timeout: options.options?.socketTimeout ?? globalConfig.socketTimeout,
2830
};
2931

3032
const agent = options.options?.https
@@ -39,6 +41,7 @@ export default class HttpConnection implements IConnectionProvider, IThriftConne
3941
agent,
4042
...this.getNodeOptions(options.options || {}),
4143
...(options.options?.nodeOptions || {}),
44+
timeout: options.options?.socketTimeout ?? globalConfig.socketTimeout,
4245
},
4346
});
4447

lib/connection/contracts/IConnectionOptions.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
export type Options = {
2+
socketTimeout?: number;
23
username?: string;
34
password?: string;
45
ssl?: boolean;

lib/contracts/IDBSQLClient.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ export interface ConnectionOptions {
1111
path: string;
1212
token: string;
1313
clientId?: string;
14+
socketTimeout?: number;
1415
}
1516

1617
export interface OpenSessionRequest {

lib/globalConfig.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
interface GlobalConfig {
22
arrowEnabled?: boolean;
33
useArrowNativeTypes?: boolean;
4+
socketTimeout: number;
45

56
retryMaxAttempts: number;
67
retriesTimeout: number; // in milliseconds
@@ -11,6 +12,7 @@ interface GlobalConfig {
1112
export default {
1213
arrowEnabled: true,
1314
useArrowNativeTypes: true,
15+
socketTimeout: 15 * 60 * 1000, // 15 minutes
1416

1517
retryMaxAttempts: 30,
1618
retriesTimeout: 900 * 1000,

patches/thrift+0.16.0.patch

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
diff --git a/node_modules/thrift/lib/nodejs/lib/thrift/http_connection.js b/node_modules/thrift/lib/nodejs/lib/thrift/http_connection.js
2-
index 17e0d0c..9e90096 100644
2+
index 17e0d0c..96cbcf1 100644
33
--- a/node_modules/thrift/lib/nodejs/lib/thrift/http_connection.js
44
+++ b/node_modules/thrift/lib/nodejs/lib/thrift/http_connection.js
55
@@ -106,7 +106,7 @@ var HttpConnection = exports.HttpConnection = function(options) {
@@ -68,7 +68,7 @@ index 17e0d0c..9e90096 100644
6868
});
6969
};
7070
};
71-
@@ -212,18 +215,33 @@ util.inherits(HttpConnection, EventEmitter);
71+
@@ -212,18 +215,41 @@ util.inherits(HttpConnection, EventEmitter);
7272
* @event {error} the "error" event is raised upon request failure passing the
7373
* Node.js error object to the listener.
7474
*/
@@ -101,9 +101,16 @@ index 17e0d0c..9e90096 100644
101101
- http.request(opts, self.responseCallback);
102102
- req.on('error', function(err) {
103103
- self.emit("error", err);
104-
- });
105104
+ https.request(opts, responseCallback) :
106105
+ http.request(opts, responseCallback);
106+
+ req.on('timeout', () => {
107+
+ // Ignore all subsequent errors on this request
108+
+ req.off('error', handleError);
109+
+ req.on('error', () => {});
110+
+ // Emit a single error and destroy request
111+
+ handleError(new thrift.TApplicationException(thrift.TApplicationExceptionType.PROTOCOL_ERROR, 'Request timed out'));
112+
+ req.destroy();
113+
});
107114
+ req.on('error', handleError);
108115
req.write(data);
109116
req.end();

tests/e2e/timeouts.test.js

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
const { expect, AssertionError } = require('chai');
2+
const config = require('./utils/config');
3+
const { DBSQLClient } = require('../..');
4+
const globalConfig = require('../../dist/globalConfig').default;
5+
6+
const openSession = async (socketTimeout) => {
7+
const client = new DBSQLClient();
8+
9+
const connection = await client.connect({
10+
host: config.host,
11+
path: config.path,
12+
token: config.token,
13+
socketTimeout,
14+
});
15+
16+
return connection.openSession({
17+
initialCatalog: config.database[0],
18+
initialSchema: config.database[1],
19+
});
20+
};
21+
22+
describe('Data fetching', () => {
23+
const query = `
24+
SELECT *
25+
FROM range(0, 100000) AS t1
26+
LEFT JOIN (SELECT 1) AS t2
27+
ORDER BY RANDOM() ASC
28+
`;
29+
30+
const socketTimeout = 1; // minimum value to make sure any request will time out
31+
32+
it('should use default socket timeout', async () => {
33+
const savedTimeout = globalConfig.socketTimeout;
34+
globalConfig.socketTimeout = socketTimeout;
35+
try {
36+
await openSession();
37+
expect.fail('It should throw an error');
38+
} catch (error) {
39+
if (error instanceof AssertionError) {
40+
throw error;
41+
}
42+
expect(error.message).to.be.eq('Request timed out');
43+
} finally {
44+
globalConfig.socketTimeout = savedTimeout;
45+
}
46+
});
47+
48+
it('should use socket timeout from options', async () => {
49+
try {
50+
await await openSession(socketTimeout);
51+
expect.fail('It should throw an error');
52+
} catch (error) {
53+
if (error instanceof AssertionError) {
54+
throw error;
55+
}
56+
expect(error.message).to.be.eq('Request timed out');
57+
}
58+
});
59+
});

tests/unit/connection/connections/HttpConnection.test.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ describe('HttpConnection.connect', () => {
162162
.then(() => {
163163
resultConnection.responseCallback({ headers: {} });
164164
expect(resultConnection.executed).to.be.true;
165-
expect(Object.keys(connection.thrift.options.nodeOptions)).to.be.deep.eq(['agent']);
165+
expect(Object.keys(connection.thrift.options.nodeOptions)).to.be.deep.eq(['agent', 'timeout']);
166166
});
167167
});
168168

0 commit comments

Comments
 (0)