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.

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

Jump to bottom

[clients] Calling .destroy() doesn't destroy the clients #2105

Closed
trivikr opened this issue Mar 4, 2021 · 3 comments · Fixed by smithy-lang/smithy-typescript#301 or #2204
Closed

[clients] Calling .destroy() doesn't destroy the clients #2105

trivikr opened this issue Mar 4, 2021 · 3 comments · Fixed by smithy-lang/smithy-typescript#301 or #2204
Assignees
@AllanZhengYP
Labels
documentation This is a problem with documentation.

Comments

@trivikr
Copy link
Member

trivikr commented Mar 4, 2021

Describe the bug

Calling .destroy() doesn't destroy the clients. I can still make calls using the clients.

Your environment

SDK version number

@aws-sdk/client-dynamodb@3.7.0

Is the issue in the browser/Node.js/ReactNative?

All, but tested in Node.js

Details of the browser/Node.js/ReactNative version

$ node -v
v14.15.4

Steps to reproduce

Code 
import { DynamoDBClient, ListTablesCommand } from "@aws-sdk/client-dynamodb";

(async () => {
  const region = "us-east-1";
  const client = new DynamoDBClient({ region });
  client.destroy();
  console.log(await client.send(new ListTablesCommand({})));
})();

Observed behavior

Prints the output of listTables command.

Expected behavior

Should throw an error since client is destroyed.

Additional context

This issue was noticed while working on destroy() functionality for Document Client #2097 (comment)
@trivikr trivikr added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 4, 2021
@trivikr trivikr mentioned this issue Mar 4, 2021
Merged
@trivikr
Copy link
Member Author

trivikr commented Mar 4, 2021

Findings: Verified by logging that destroy() is called on httpAgent and httpsAgent in NodeHttpHandler when client.destroy() is called.

aws-sdk-js-v3/packages/node-http-handler/src/node-http-handler.ts

Lines 50 to 53 in c612c75

destroy(): void {
this.httpAgent.destroy();
this.httpsAgent.destroy();
}

As per Node.js docs, the agent.destroy() should destroy the sockets that are currently in use by the agent https://nodejs.org/api/http.html#http_agent_destroy

@trivikr trivikr removed the needs-triage This issue or PR still needs to be triaged. label Mar 4, 2021
@trivikr
Copy link
Member Author

trivikr commented Mar 4, 2021

In Node.js, the request goes through even if .destroy() is called on agent passed to the request.

Code 
const http = require("http");

const agent = new http.Agent({ keepAlive: true });
const options = {
  hostname: "example.com",
  method: "GET",
  agent,
};

agent.destroy();

const req = http.request(options, (res) => {
  console.log(`statusCode: ${res.statusCode}`);

  res.on("data", (d) => {
    process.stdout.write(d);
  });
});

req.on("error", (error) => {
  console.error(error);
});

req.end();
Output 
statusCode: 200
<!doctype html>
<html>
<head>
...

Tested in the following Node.js versions:

  • v10.23.1
  • v12.20.1
  • v14.15.4

@AllanZhengYP AllanZhengYP added documentation This is a problem with documentation. and removed bug This issue is a bug. labels Mar 31, 2021
@AllanZhengYP AllanZhengYP self-assigned this Mar 31, 2021
This was referenced Mar 31, 2021
Merged
Merged
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@AllanZhengYP AllanZhengYP

Labels
documentation This is a problem with documentation.
Projects
None yet
Milestone
No milestone
2 participants
@AllanZhengYP @trivikr