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

Unclosed Resources (timers and socket) #90

Open
geoffhendrey opened this issue Sep 27, 2024 · 11 comments
Open

Unclosed Resources (timers and socket) #90

geoffhendrey opened this issue Sep 27, 2024 · 11 comments

Comments

@geoffhendrey
Copy link
Contributor

geoffhendrey commented Sep 27, 2024

First, thanks for making this library. It is great to have a native JS client for Pulsar to use in Bun.js, especially because the pulsar-client does not work (hard to diagnose failure) in Bun.js

Describe the bug
A Jest test (essentially a cut-n-paste of the example in README.md) leaves dangling resources. This manifests itself in 2 ways:

  1. calling node --experimental-vm-modules node_modules/jest/bin/jest.js Pulsar.test.js --detectOpenHandles results in the program never finishing, and node reporting open handles like this:
Jest has detected the following 1 open handle potentially keeping Jest from exiting:

  ●  Timeout

      82 |     test('should produce and consume messages successfully', async () => {
      83 |         // Send a batch of messages
    > 84 |         await producer.sendBatch({ messages: messagesToSend });
         |                        ^
      85 |
      86 |         // Wait for messages to be received
      87 |         const maxWaitTime = 10000; // 10 seconds

      at node_modules/pulsar-flex/src/responseMediators/abstract/responseMediator.js:38:7
      at SendResponseMediator.response (node_modules/pulsar-flex/src/responseMediators/abstract/responseMediator.js:35:12)
      at sendRequest (node_modules/pulsar-flex/src/client/network/connection.js:10:27)
      at sendPayloadBatchCommandRequest (node_modules/pulsar-flex/src/client/network/connection.js:43:14)
      at Object.sendBatch (node_modules/pulsar-flex/src/producer/services/sendBatch.js:23:16)
      at Producer.sendBatch (node_modules/pulsar-flex/src/producer/index.js:186:42)
      at Object.<anonymous> (src/test/Pulsar.test.js:84:24)

this is because --detectOpenHandles is finding a Timeout that has not fired. This is due to this line where a Timeout 10 seconds is setup to reject the promise in case there is never a resolve() (when autoResolve is false).

THis is not in itself a huge problem, it can be worked around by simply waiting at least 10 seconds yourself before allowing the Jest test to finish (i.e. inserting this at the end of your test method: await new Promise(resolve => setTimeout(resolve, 11000));)

  1. Calling producer.close() does not actually call producer._client.getCnx().close(); Consequently the producer socket remains open and 'ping' and 'pong' continue indefinitely. And the program never exits.

Reproduce

  1. download this PulsarFlex.test.js into any node project you are working on (in the command below I assume Jest is located at node_modules/jest/bin/jest.js)
  2. run node --experimental-vm-modules node_modules/jest/bin/jest.js Pulsar.test.js --detectOpenHandles
  3. observe that node never returns, and jest reports open files

Expected behavior
Test immediately passes and Node exits normally. No open handles detected.

Observed behavior

  • test passes...BUT...
  • Open handles detected
  • node never exits
  • 'ping' and 'pong' events cause logger to fail because Jest does not allow logging after test has completed

Environment:

  • OS: [ Mac OS 14.3]
  • PulsarFlex version [1.1.1]
  • Pulsar version [Pulsar-Java-v3.3.1]
  • NodeJS version [22.5.1]

Additional context
Congrats, you are the only option for accessing Pulsar from Bun.js because the pulsar-client fails in Bun.js.
You can find the solution to this bug by uncommenting two lines at the bottom of this test. To incorporate those into product, you want to make sure you close the connection from within Producer.close, and you will want a way to preemptively kill this timeout when the Producer closes (otherwise Jest will still report there is an open 'handle' (a Timeout) when a test exits, even when the test has gracefully closed the producer before finishing.

geoffhendrey added a commit to cisco-open/stated that referenced this issue Sep 27, 2024
* introduce CliCoreBase.ts to remove all Node repl dependencies, allowing future REPL to be built without using Node REPL. Small change to stringify so that Timer objects are recognized in Bun. Add couple Bun detectors in tests to work around Jest specialties that are not availavle in Bun. Add github actions so that all tests run on Bun as well as node.

* fix bun:run script

* make workaround for bug in pulsar-flex. Issue submitted: ayeo-flex-org/pulsar-flex#90
geoffhendrey added a commit to cisco-open/stated that referenced this issue Sep 27, 2024
* introduce CliCoreBase.ts to remove all Node repl dependencies, allowing future REPL to be built without using Node REPL. Small change to stringify so that Timer objects are recognized in Bun. Add couple Bun detectors in tests to work around Jest specialties that are not availavle in Bun. Add github actions so that all tests run on Bun as well as node.

* fix bun:run script

* make workaround for bug in pulsar-flex. Issue submitted: ayeo-flex-org/pulsar-flex#90

* v0.1.35
@geoffhendrey
Copy link
Contributor Author

is anyone active on this project to triage issues?

@galrose
Copy link
Collaborator

galrose commented Oct 4, 2024

Yes sorry I missed this, would you like to open a PR for this?

@geoffhendrey
Copy link
Contributor Author

sure thing. Not sure I can get to it this weekend but will do.

@geoffhendrey
Copy link
Contributor Author

I have a pr @galrose, can i get permission to open a PR?

@geoffhendrey
Copy link
Contributor Author

@galrose can i get permission to push a PR? cc @danielsinai

@galrose
Copy link
Collaborator

galrose commented Oct 30, 2024

@geoffhendrey sorry we had a holiday here, did you try to fork and open a PR from there?

geoffhendrey added a commit to geoffhendrey/pulsar-flex that referenced this issue Oct 31, 2024
@geoffhendrey
Copy link
Contributor Author

#91

@geoffhendrey
Copy link
Contributor Author

@galrose do you think the test is failing because /bin/sh: 1: docker-compose: not found. I would love to get this PR in! Is there some way I can help, or do you think the failure is due to my change?

@geoffhendrey
Copy link
Contributor Author

spamming everyone because I cannot get any response from anyone on this project @danielsinai @galrose @ronfarkash @sOfekS

@danielsinai
Copy link
Collaborator

Hi @geoffhendrey sry for this, I will make sure its handled tomorrow.

@geoffhendrey
Copy link
Contributor Author

please publish a new version to NPM so we can pickup the changes in the merged branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants