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

test: simplify the test cases in http timeouts mix #43470

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 49 additions & 71 deletions test/parallel/test-http-server-request-timeouts-mixed.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const common = require('../common');
const assert = require('assert');
const { createServer } = require('http');
const { connect } = require('net');
const { promisify } = require('util');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const { promisify } = require('util');
const { setTimeout: delay } = require('timers/promises');


// This test validates that request are correct checked for both requests and headers timeout in various situations.

Expand All @@ -16,10 +17,13 @@ const responseTimeout = 'HTTP/1.1 408 Request Timeout\r\n';

const headersTimeout = common.platformTimeout(2000);
const connectionsCheckingInterval = headersTimeout / 4;
const requestTimeout = headersTimeout * 2;
const threadSleepDelay = requestTimeout + headersTimeout;
const delay = promisify(setTimeout);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const delay = promisify(setTimeout);


const server = createServer({
headersTimeout,
requestTimeout: headersTimeout * 2,
requestTimeout,
keepAliveTimeout: 0,
connectionsCheckingInterval
}, common.mustCall((req, res) => {
Expand All @@ -32,7 +36,7 @@ const server = createServer({
}, 4));

assert.strictEqual(server.headersTimeout, headersTimeout);
assert.strictEqual(server.requestTimeout, headersTimeout * 2);
assert.strictEqual(server.requestTimeout, requestTimeout);

let i = 0;
function createClient(server) {
Expand All @@ -58,74 +62,48 @@ function createClient(server) {
return request;
}

server.listen(0, common.mustCall(() => {
server.listen(0, common.mustCall(() => (async () => {
// Create first and second request and set headers
const request1 = createClient(server);
let request2;
let request3;
let request4;
let request5;

// Send the first request and stop before the body
const request2 = createClient(server);
request1.client.write(requestBodyPart1);

// After a little while send two new requests
setTimeout(() => {
request2 = createClient(server);
request3 = createClient(server);

// Send the second request, stop in the middle of the headers
request2.client.write(requestBodyPart1);
// Send the second request, stop in the middle of the headers
request3.client.write(requestBodyPart1);
}, headersTimeout * 0.2);

// After another little while send the last two new requests
setTimeout(() => {
request4 = createClient(server);
request5 = createClient(server);

// Send the fourth request, stop in the middle of the headers
request4.client.write(requestBodyPart1);
// Send the fifth request, stop in the middle of the headers
request5.client.write(requestBodyPart1);
}, headersTimeout * 0.6);

setTimeout(() => {
// Finish the first request
request1.client.write(requestBodyPart2 + requestBodyPart3);

// Complete headers for all requests but second
request3.client.write(requestBodyPart2);
request4.client.write(requestBodyPart2);
request5.client.write(requestBodyPart2);
}, headersTimeout * 0.8);

setTimeout(() => {
// After the first timeout, the first request should have been completed and second timedout
assert(request1.completed);
assert(request2.completed);
assert(!request3.completed);
assert(!request4.completed);
assert(!request5.completed);

assert(request1.response.startsWith(responseOk));
assert(request2.response.startsWith(responseTimeout)); // It is expired due to headersTimeout
}, headersTimeout * 1.2 + connectionsCheckingInterval);

setTimeout(() => {
// Complete the body for the fourth request
request4.client.write(requestBodyPart3);
}, headersTimeout * 1.5);

setTimeout(() => {
// All request should be completed now, either with 200 or 408
assert(request3.completed);
assert(request4.completed);
assert(request5.completed);

assert(request3.response.startsWith(responseTimeout)); // It is expired due to requestTimeout
assert(request4.response.startsWith(responseOk));
assert(request5.response.startsWith(responseTimeout)); // It is expired due to requestTimeout
server.close();
}, headersTimeout * 3 + connectionsCheckingInterval);
}));
request2.client.write(requestBodyPart1);

// Finish the first request now and wait more than the request timeout value
request1.client.write(requestBodyPart2 + requestBodyPart3);
await delay(threadSleepDelay);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding this changes the original test. request3 and request4 are created after the requestTimeout expired.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request3 and request4 are created at request1 creation time + threadSleepDelay, where threadSleepDelay = requestTimeout + headersTimeout.

It's not like this in the original test. All request are created in a requestTimeout interval in the original one. There are 4 concurrent requests. Now there are 2.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. But, basically we are testing 5 requests header timeout. In the previous pattern, we are doing it using some setTimeout functions and it was causing the flaky.
So, rewritten that to this way.

Copy link
Member

@lpinca lpinca Jun 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each of these tests already has a standalone version. This one was made to test concurrent requests. If we have to change it like this, it is better to remove it completely as per https://github.com/nodejs/node/pull/42893/files#r861466882.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Bu, as per the conversation, PR owner telling that he added this case to make concurrent requests and testing them together.

Previously, 5 concurrent requests are creating. Now, we are creating 2 concurrent requests at a time. Again, we are creating 3 concurrent requests.

This splitting is to avoid flakiness.

Instead of removing, I prefer to have some test for concurrency at least. I appreciate that you already forecasted the flakiness in #42893 . Good work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but I'm -1 to change the test like this. Again, this changes the original intentions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, this test should have 4 concurrent request to properly test the possible combinations.
We can revisit the timeout schedule (and eventually use async/await for readable, but please do not change the order of creation.


// First and second request should complete now
assert(request1.completed);
assert(request2.completed); // Completed, but a timeout request

assert(request1.response.startsWith(responseOk));
assert(request2.response.startsWith(responseTimeout)); // Reques expired due to headersTimeout

// Create another 3 requests with headers
const request3 = createClient(server);
const request4 = createClient(server);
const request5 = createClient(server);
request3.client.write(requestBodyPart1 + requestBodyPart2);
request4.client.write(requestBodyPart1 + requestBodyPart2);
request5.client.write(requestBodyPart1 + requestBodyPart2);

// None of the requests will be completed because they only recieved headers
assert(!request3.completed);
assert(!request4.completed);
assert(!request5.completed);

// Finish the fourth request now and wait more than the request timeout value
request4.client.write(requestBodyPart3);
await delay(threadSleepDelay);

// All three requests should complete now
assert(request3.completed);
assert(request4.completed);
assert(request5.completed);

assert(request3.response.startsWith(responseTimeout)); // It is expired due to headersTimeout
assert(request4.response.startsWith(responseOk));
assert(request5.response.startsWith(responseTimeout)); // It is expired due to headersTimeout
server.close();
})().then(common.mustCall())));