Eppo-exp / node-server-sdk

Eppo Node.js SDK
MIT License
9 stars 0 forks source link

Pavel/epd 1022 oom #22

Closed pavelflux closed 1 year ago

pavelflux commented 1 year ago

labels: mergeable

Fixes: EPD-1022

Motivation and Context

There is quite a complex issue with OOM, best seen in e2e tests of the main repo. For this to be fixed, we need to be able to stop polling manually via the EppoSDKClient.

Description

After adding eppo sdk client to api e2e tests in main repo (in this PR) we started to hit OOM error (while running jest, locally or in GH Actions). After some research I found out that eppo sdk is causing this issue. My logic here is:

  ●  Cannot log after tests are done. Did you forget to wait for something async in your test?
    Attempted to log "Error polling configurations: connect ECONNREFUSED ::1:4000".

      at console.error (node_modules/@jest/console/build/BufferedConsole.js:127:10)
      at Timeout.poll [as _onTimeout] (node_modules/@eppo/node-server-sdk/src/poller.ts:27:15)

  ●  Cannot log after tests are done. Did you forget to wait for something async in your test?
    Attempted to log "Error polling configurations: connect ECONNREFUSED ::1:4000".

      at console.error (node_modules/@jest/console/build/BufferedConsole.js:127:10)
      at Timeout.poll [as _onTimeout] (node_modules/@eppo/node-server-sdk/src/poller.ts:27:15)

  ●  Cannot log after tests are done. Did you forget to wait for something async in your test?
    Attempted to log "Error polling configurations: connect ECONNREFUSED ::1:4000".

      at console.error (node_modules/@jest/console/build/BufferedConsole.js:127:10)
      at Timeout.poll [as _onTimeout] (node_modules/@eppo/node-server-sdk/src/poller.ts:27:15)

even if this does not lead to OOM issue itself, it prevents Jest from exiting. (Jest has an issue with OOM, which I fixed in the PR (ADD LINK TO PR HERE)

Also, please note that recursive setTimeout itself is OOM-prone, so if it was not for the jitter, I would replace it with setInterval.

@leoromanovsky @chasdevs maybe we can have another condition to stop the recursion?

How has this been tested?

Manually and via tests