CLDSRV-716 Disable default timeout#5859
Conversation
Hello benzekrimaha,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files
@@ Coverage Diff @@
## development/9.0 #5859 +/- ##
===================================================
- Coverage 75.88% 75.85% -0.04%
===================================================
Files 188 188
Lines 11969 11970 +1
===================================================
- Hits 9083 9080 -3
- Misses 2886 2890 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
benzekrimaha
force-pushed
the
bugfix/CLDSRV-716-fix-default-timeout
branch
from
6a441f7 to
b0d52c3
Compare
| } | ||
|
|
||
| server.requestTimeout = 0; // disabling request timeout | ||
|
|
There was a problem hiding this comment.
Then I suggest adding a unit test:
const assert = require('assert');
const sinon = require('sinon');
const http = require('http');
const https = require('https');
const promClient = require('prom-client');
describe('S3Server request timeout', () => {
let sandbox;
let mockServer;
beforeEach(() => {
sandbox = sinon.createSandbox();
// Clear prometheus registry to avoid conflicts
promClient.register.clear();
// Create a mock server to capture the requestTimeout setting
mockServer = {
requestTimeout: null,
on: sandbox.stub(),
listen: sandbox.stub(),
address: sandbox.stub().returns({ address: '127.0.0.1', port: 8000 }),
};
// Mock server creation to return our mock
sandbox.stub(http, 'createServer').returns(mockServer);
sandbox.stub(https, 'createServer').returns(mockServer);
});
afterEach(() => {
sandbox.restore();
});
it('should set server.requestTimeout to 0 when starting server', () => {
const { S3Server } = require('../../lib/server');
const server = new S3Server();
// Call _startServer which should set requestTimeout = 0
server._startServer(() => {}, 8000, '127.0.0.1');
// Verify that requestTimeout was set to 0
assert.strictEqual(mockServer.requestTimeout, 0);
});
});You'll need to also export the s3Server class from server.js
module.exports.S3Server = S3Server;I think this is important to avoid missing such issue again in the future.
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
Starting node 18 we now have a default timeout of 300000 ( 5 minutes) set , which is not suitable for our use cases. This commit disables the default timeout by setting the server.requestTimeout to 0 https://nodejs.org/api/http.html#serverrequesttimeout Issue: CLDSRV-716
benzekrimaha
force-pushed
the
bugfix/CLDSRV-716-fix-default-timeout
branch
4 times, most recently
from
1163014 to
17599ec
Compare
benzekrimaha
force-pushed
the
bugfix/CLDSRV-716-fix-default-timeout
branch
from
17599ec to
d937714
Compare
|
/create_integration_branches |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
|
@bert-e create_pull_requests |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
Follow integration pull requests if you would like to be notified of The following options are set: create_pull_requests, create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_pull_requests, create_integration_branches |
|
/approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-716. Goodbye benzekrimaha. The following options are set: approve, create_pull_requests, create_integration_branches |
3 participants
Issue: CLDSRV-716