ayeo-flex-org / pulsar-flex

Pulsar Flex is a modern Apache Pulsar client for Node.js, developed to be independent of C++.
MIT License
45 stars 9 forks source link

Unclosed Resources (timers and socket) #90

Open geoffhendrey opened 1 month ago

geoffhendrey commented 1 month ago

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));)

2) 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

Environment:

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 commented 1 month ago

is anyone active on this project to triage issues?

galrose commented 1 month ago

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

geoffhendrey commented 1 month ago

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

geoffhendrey commented 2 weeks ago

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

geoffhendrey commented 2 weeks ago

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

galrose commented 1 week ago

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

geoffhendrey commented 1 week ago

https://github.com/ayeo-flex-org/pulsar-flex/pull/91