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

AbortSignal overwrites onabort rather than using addEventListener, preventing multiple use #4872

Closed
3 tasks done
u873838 opened this issue Jun 22, 2023 · 4 comments · Fixed by #4895, smithy-lang/smithy-typescript#801 or smithy-lang/smithy-typescript#1308
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue 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

Comments

@u873838
Copy link

u873838 commented Jun 22, 2023

Checkboxes for prior research

Describe the bug

The AbortSignal interface (spec) defines AbortSignal as extending the EventTarget interface, which provides the addEventListener method for defining event listeners.

When an AbortSignal is passed to client calls, the onabort method is used instead for signaling aborts. This is unexpected, as this behaves differently than standard functions that accept AbortSignal arguments. It also prevents the same AbortSignal from being used for simultaneous SDK calls.

SDK version number

@aws-sdk/[email protected],@aws-sdk/[email protected]

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

v20.3.0

Reproduction Steps

const { Agent } = require('node:http');
const { Lambda } = require('@aws-sdk/client-lambda');
const { NodeHttpHandler } = require("@aws-sdk/node-http-handler");
const express = require('express');

(async () => {
  const app = express();
  app.use('/delay', (req, res) => {
    setTimeout(() => {
      res.json({});
    }, 3_000);
  });

  await new Promise((resolve) => {
    app.listen(8080, '127.0.0.1', () => {
      resolve();
    });
  });

  const signal = AbortSignal.timeout(2_000);
  signal.addEventListener('abort', () => {
    console.log('outer abort handler 1');
  });

  const  l = new Lambda({
    region: 'us-west-2',
    maxAttempts: 1,
    endpoint: 'http://127.0.0.1:8080/delay',
    requestHandler: new NodeHttpHandler({
      httpAgent: new Agent({}),
    }),
  });
  console.log(await Promise.allSettled([
    l.invoke({
      FunctionName: 'test-fn',
    }, {
      abortSignal: signal,
    }),
    l.invoke({
      FunctionName: 'test-fn',
    }, {
      abortSignal: signal,
    }),
  ]));
})();

Observed Behavior

outer abort handler 1
[
  {
    status: 'fulfilled',
    value: {
      '$metadata': [Object],
      Payload: [Uint8ArrayBlobAdapter [Uint8Array]],
      StatusCode: 200
    }
  },
  {
    status: 'rejected',
    reason: Error [AbortError]: Request aborted
        at node/node_modules/@aws-sdk/node-http-handler/dist-cjs/node-http-handler.js:66:36
        at new Promise (<anonymous>)
        at NodeHttpHandler.handle (node/node_modules/@aws-sdk/node-http-handler/dist-cjs/node-http-handler.js:51:16)
        at node/node_modules/@aws-sdk/client-lambda/dist-cjs/commands/InvokeCommand.js:38:58
        at node/node_modules/@aws-sdk/middleware-serde/dist-cjs/deserializerMiddleware.js:5:32
        at node/node_modules/@aws-sdk/middleware-signing/dist-cjs/awsAuthMiddleware.js:14:26
        at async node/node_modules/@aws-sdk/middleware-retry/dist-cjs/retryMiddleware.js:27:46
        at async node/node_modules/@aws-sdk/middleware-logger/dist-cjs/loggerMiddleware.js:7:26
        at async Promise.allSettled (index 1)
        at async node/index.js:33:15 {
      '$metadata': [Object]
    }
  }
]

Expected Behavior

I expected both .invoke calls to fail with a "Request aborted" error.

Possible Solution

Update the internal AbortSignal type to extend EventTarget, and use addEventListener throughout the SDK to register abort handlers.

Additional Information/Context

No response

@u873838 u873838 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 22, 2023
@RanVaknin RanVaknin self-assigned this Jun 27, 2023
@RanVaknin RanVaknin added pending-release This issue will be fixed by an approved PR that hasn't been released yet. and removed needs-triage This issue or PR still needs to be triaged. labels Jun 29, 2023
@u873838
Copy link
Author

u873838 commented Jun 30, 2023

@RanVaknin
Can you please reopen the ticket? I checked out the latest commit (a812fc8) and the issue is not fixed.

@trivikr trivikr reopened this Jun 30, 2023
@RanVaknin RanVaknin added p2 This is a standard priority issue and removed pending-release This issue will be fixed by an approved PR that hasn't been released yet. labels Sep 13, 2023
@kuhe kuhe added the queued This issues is on the AWS team's backlog label Jun 14, 2024
@kuhe kuhe added the pending-release This issue will be fixed by an approved PR that hasn't been released yet. label Jun 18, 2024
@kuhe kuhe self-assigned this Jun 18, 2024
@kuhe kuhe reopened this Jun 18, 2024
@kuhe
Copy link
Contributor

kuhe commented Jun 18, 2024

Using the latest AWS SDK and doing a fresh install overwriting any existing lockfiles to get version 3.1.0 of the @smithy/node-http-handler or @smithy/fetch-http-handler should enable the use of addEventListener for abortSignals.

@kuhe kuhe closed this as completed Jun 18, 2024
@kuhe
Copy link
Contributor

kuhe commented Jun 18, 2024

in a future update to the AWS SDK for JavaScript (v3), we will raise the minimum version of those http-handlers to bring in the fix automatically as well.

Copy link

github-actions bot commented Jul 3, 2024

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 Jul 3, 2024
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 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
Projects
None yet
4 participants