awslabs / aws-crt-nodejs

NodeJS bindings for the AWS Common Runtime.
Apache License 2.0
40 stars 27 forks source link

aws_logger handle is not closed while running jest tests. #291

Open Sam1540 opened 2 years ago

Sam1540 commented 2 years ago

While running jest for my nodejs tests, I am noticing that the "aws_logger" handle is not getting closed resulting in automation tests failing.

Jest has detected the following 1 open handle potentially keeping Jest from exiting:

● aws_logger

  1 | const {PromiseSocket, TimeoutError} = require("promise-socket");
> 2 | const { mqtt, io, iot } = require('aws-crt');
    |                           ^
  3 | const fs = require( "fs" );
  4 | const { loginUser, logoutUser } = require( "./aws-cognito" );
  5 | const cfg = require( './aws-config.json' )

  at Runtime._loadModule (node_modules/jest-runtime/build/index.js:1180:29)
  at Object.<anonymous> (node_modules/aws-crt/lib/native/binding.js:40:19)
  at Object.<anonymous> (node_modules/aws-crt/lib/native/crt.ts:22:1)
  at Object.<anonymous> (node_modules/aws-crt/lib/index.ts:18:1)
  at Object.<anonymous> (tests/server.spec.js:2:27)
  at TestScheduler.scheduleTests (node_modules/@jest/core/build/TestScheduler.js:333:13)
  at runJest (node_modules/@jest/core/build/runJest.js:404:19)
  at _run10000 (node_modules/@jest/core/build/cli/index.js:320:7)
  at runCLI (node_modules/@jest/core/build/cli/index.js:173:3)

error Command failed with exit code 1.

emfleisch commented 2 years ago

From what I have seen this is still an issue with aws-crt, however I was able to mitigate by updating my aws-sdk clients to version 3.56.0 (@aws-sdk/client-lambda, @aws-sdk/client-s3) which updated theirs dependency on signature-v4-crt, which has subsequently removed its dep of this library. I got lucky with 3.56.0 getting released the day I encountered this problem, as I was not able to get around it in code or with jest settings.

blesson3 commented 2 years ago

Can confirm that updating my @aws-sdk/client-s3 dependency resolved this issue. Couldn't find another way around it.

@aws-sdk/client-s3: 3.52.0 (broken) => 3.58.0 (fixed)
mikelpr commented 2 years ago

this is still a problem on aws-crt

oceanofmaya commented 2 years ago

the aws sdk clients I use are at version 3.118.0 and aws-crt version 1.12.5 which are the latest available when I'm commenting and I still see this issue.

UPDATE: I was able to resolve it by switching jest spies I had for sns client with a mock client using aws-sdk-client-mock

nponeccop commented 1 year ago

I had a similar issue, and found a somewhat strange solution. It may not be the best solution but at least something that works.

I had global code in a module doing something like this:

import AWSXRay from 'aws-xray-sdk-core'
import { NodeHttpHandler } from '@aws-sdk/node-http-handler'
import { DynamoDBClient } from '@aws-sdk/client-dynamodb'

AWSXRay.setContextMissingStrategy('LOG_ERROR')
const client = new DynamoDBClient({
  maxAttempts: 10,
  requestHandler: new NodeHttpHandler({
    connectionTimeout: 50,
    socketTimeout: 50
  })
})
// throw new Error("Oh no!")

And if you uncomment the throw line there, it throws during tests. It means that a new DynamoDB client is created in tests, ultimately leading to the aws_logger problem in this issue. It also can be independently confirmed by running tests under a debugger and putting a breakpoint (debugging Jest this way works out of the box in JetBrains WebStorm).

So, is there a way to prevent the execution of this code in tests? It turns there is. You need to do 2 things:

I hope it helps someone. I couldn't avoid the throwing until I used __mocks__, so it's essential.

nponeccop commented 1 year ago

UPDATE: I was able to resolve it by switching jest spies I had for sns client with a mock client using aws-sdk-client-mock

aws-sdk-client-mock uses sinon for mocking. It works much better than the built-in Jest mocks unfortunately, and doesn't suffer from pseudo-calls, strange preprocessing and other Jest peculiarities that could be interpreted as design flaws. Unfortunately, Jest is the most popular testing framework out there. If you use the excellent JetBrains IDEs it's one of 3 testing framework choices, so people will keep hitting these issues no matter what, just because Jest mocking is inadequate.

bretambrose commented 1 year ago

The logger handle is closed when the NAPI module is finalized by node. We don't have any control of when or if that happens; it seems to be a property of garbage collection. So on the surface, there's not much that can be done about the logger if we keep existing behavior.

However, there is a possibility that instead of enabling the logging router (takes native logging and writes to the Javascript console api) by default, we could instead make it opt-in (by environment and/or API call). This is potentially a disruptive change and I'm unsure of all of the consequences of it yet but I don't see any other way forward with this issue.

Giovanni-Ilacqua commented 8 months ago

I had a similar issue, and found a somewhat strange solution. It may not be the best solution but at least something that works.

I had global code in a module doing something like this:

import AWSXRay from 'aws-xray-sdk-core'
import { NodeHttpHandler } from '@aws-sdk/node-http-handler'
import { DynamoDBClient } from '@aws-sdk/client-dynamodb'

AWSXRay.setContextMissingStrategy('LOG_ERROR')
const client = new DynamoDBClient({
  maxAttempts: 10,
  requestHandler: new NodeHttpHandler({
    connectionTimeout: 50,
    socketTimeout: 50
  })
})
// throw new Error("Oh no!")

And if you uncomment the throw line there, it throws during tests. It means that a new DynamoDB client is created in tests, ultimately leading to the aws_logger problem in this issue. It also can be independently confirmed by running tests under a debugger and putting a breakpoint (debugging Jest this way works out of the box in JetBrains WebStorm).

So, is there a way to prevent the execution of this code in tests? It turns there is. You need to do 2 things:

  • create a mock using the __mocks__ folder
  • add this module to the mock list using jest.mock pseudo-call in your tests (it looks like a normal call but it isn't).

I hope it helps someone. I couldn't avoid the throwing until I used __mocks__, so it's essential.

I am struggling with this same issue. I tried creating a mocks directory and adding a file to it named aws_logger.ts which exports a jest function

export const aws_logger = jest.fn(() => 'Mocked aws_logger');

The in the test I call jest.mock()

jest.mock('../../../../__mocks__/aws_logger', () => ({
  aws_logger: jest.fn(),
}));

But the warning is persisting... so I must be doing something incorrectly. Could you share some more information about how you managed to get jest to use the mocked node module?

wbeardall commented 3 months ago

This is still an issue with SDK clients version 3.583.0, and aws-crt version 1.21.2. Any chance it might get fixed?

bretambrose commented 3 months ago

The status of this is unchanged from my previous comment on the issue.