Skip to content

test: close all connections to prevent server hang #47038

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.

Sign up for GitHub

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

Closed
abmusse wants to merge 1 commit into from
Closed

test: close all connections to prevent server hang #47038

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions test/parallel/test-http-pipeline-flood.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,15 @@ function parent() {
const args = [__filename, 'child', this.address().port];
const child = spawn(process.execPath, args, { stdio: 'inherit' });
child.on('close', common.mustCall(function() {
if (common.isIBMi) {
// On IBM i calling server.close after the child process closes
// is not enough to end the server. For some reason the connection still
// lingers around with the server even though the process was closed.
// Calling closeAllConnections cleans up the lingering connection.
// Then calling server.close stops the server from listening
// for new connections and ends the server.
server.closeAllConnections();

@bnoordhuis bnoordhuis Mar 12, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this invalidates the test. The child process gets killed after 200 ms and that should result in the TCP connections getting terminated. Does that happen on ibm i?

@abmusse abmusse Mar 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

After 200 ms the child process gets killed but the client connection does not get destroyed. After the first timeout, the server timeouts a second time and tries to kill the already killed child process. Then the server just hangs as its no longer listening for new connections but still keeps handle to the defunct child process connection. Which I believe prevents the process from ending causing a timeout in the test runner.

Please see #47038 (comment) as I documented the different debug output from testing on Linux, AIX, and IBM i.

I want to know:

  • Why the child process connection does not auto destroy after the child is killed?
  • What code path handles that?
  • Why is that not being called in the IBM i case?

Any hints in the right direction to these questions would be greatly appreciated!

@bnoordhuis bnoordhuis Mar 13, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The test expects the operating system to clean up the child process's TCP connections when it's terminated. That in turn should cause the TCP connections in the parent process to hang up with e.g. a ECONNRESET error.

If that doesn't happen, then the problem is probably somewhere lower in the stack.

@abmusse abmusse Mar 13, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think the root cause is the child process connection not getting cleaned up on termination. I will need to further investigate.

@abmusse abmusse Mar 13, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On the Node.js side is there a way to get the TCP connection status?

@bnoordhuis bnoordhuis Mar 13, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you mean like a getsockopt() option: no.

}
server.close();
}));

Expand Down