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

[node-http-handler] 100 continue promise prevents node process exit #4769

Closed
3 tasks done
dominique-pfister opened this issue May 30, 2023 · 5 comments
Closed
3 tasks done
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue

Comments

@dominique-pfister
Copy link

dominique-pfister commented May 30, 2023

Checkboxes for prior research

Describe the bug

After updating to @aws-sdk/node-http-handler version 3.341.0, we suddenly experience test runs that hang for a couple of seconds, after all tests have been executed.

SDK version number

@aws-sdk/node-http-handler@3.341.0

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

v18.16.0

Reproduction Steps

import assert from 'assert';
import nock from 'nock';
import { HttpRequest } from '@aws-sdk/protocol-http';
import { NodeHttpHandler } from '@aws-sdk/node-http-handler';

describe('Test exposing setTimeout blocking test exit', () => {
  it('can handle expect 100-continue', async () => {
    nock('http://localhost:8080/')
      .put('/')
      .reply(201);
    const body = Buffer.from('test');
    const nodeHttpHandler = new NodeHttpHandler({ requestTimeout: 15000 });
    const { response } = await nodeHttpHandler.handle(
      new HttpRequest({
        hostname: 'localhost',
        method: 'PUT',
        port: 8080,
        protocol: 'http:',
        path: '/',
        headers: {
          Expect: '100-continue',
        },
        body,
      }),
      {},
    );
    assert.strictEqual(response.statusCode, 201);
  });
});

Observed Behavior

Executing that test with $ npx mocha node-http-handler.test.js shows a successful test run, but the node process hangs for the number of milliseconds specified in the requestTimeout above. Same happens when 100 Continue is actually returned by the server.

Expected Behavior

After the test has executed, the node process should exit.

Possible Solution

In

one could save the setTimeout return value (a timeout handle) and clear the timeout after the continue event has been received:

if (expect === "100-continue") {
    let timeout;
    await Promise.race<void>([
      new Promise((resolve) => {
        timeout = setTimeout(resolve, Math.max(MIN_WAIT_TIME, maxContinueTimeoutMs));
      }),
      new Promise((resolve) => {
        httpRequest.on("continue", () => {
          if (timeout) {
            clearTimeout(timeout);
          }
          resolve();
        });
      }),
    ]);
  }

Additional Information/Context

No response

@dominique-pfister dominique-pfister added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 30, 2023
@dominique-pfister dominique-pfister changed the title 100 continue promise prevents node shutdown [node-http-handler] 100 continue promise prevents node process exit May 30, 2023
@kuhe kuhe self-assigned this May 31, 2023
@kuhe kuhe added queued This issues is on the AWS team's backlog p2 This is a standard priority issue and removed needs-triage This issue or PR still needs to be triaged. labels May 31, 2023
@bcshmily
Copy link

bcshmily commented Jun 1, 2023

In AwsBatch, use client-s3+lib-storage to upload files,
The program will wait for 15 minutes before ending

Have problem in ver 3.342.0, work fine in ver3.329.0

It should be related to this update

 new S3Client({
      apiVersion: "2006-03-01",
      requestHandler: new NodeHttpHandler({
        socketTimeout: 15 * 60 * 1000
      }),
    });

@kuhe
Copy link
Contributor

kuhe commented Jun 1, 2023

this fix is expected to be published later today as https://github.com/aws/aws-sdk-js-v3/releases/tag/v3.344.0

@kuhe kuhe added the pending-release This issue will be fixed by an approved PR that hasn't been released yet. label Jun 1, 2023
@kuhe kuhe removed pending-release This issue will be fixed by an approved PR that hasn't been released yet. queued This issues is on the AWS team's backlog labels Jun 2, 2023
@bcshmily
Copy link

bcshmily commented Jun 7, 2023

@kuhe
I have reported a new issue about 100-continue(#4799).
Can you take some time to take a look?

@kuhe
Copy link
Contributor

kuhe commented Jun 8, 2023

#4805 is for #4799

@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 Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

3 participants