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

Unreference timer created in parseWorkflowCode #1370

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

jhecking
Copy link
Contributor

@jhecking jhecking commented Mar 5, 2024

What was changed

Unreference timer created in parseWorkflowCode which can prevent Node.js from closing the event loop until the timer has fired.

Why?

The parseWorkflowCode function creates a 10 second timeout to keep the script and context from getting garbage collected. For normal usage that timeout is not a problem, since workers are expected to run for longer than 10 seconds anyway.

However, when running unit tests these timers can keep the Node.js event loop from shutting down even after the tests have completed successfully, if the tests complete in less than 10 seconds. That's especially common when running individual tests cases rather than the entire test suite.

The fix is rather simple. We just need to "unreference" the timer, to prevent it from keeping the event loop alive unnecessarily.

Running a single "replay history" workflow test without this fix:

  services
Preloading source map parsing context
2024-03-05T08:14:53.436024Z  INFO temporal_sdk_core: Registering replay worker task_queue="test"
2024-03-05T08:14:53.436079Z  INFO temporal_sdk_core::worker: Initializing worker task_queue=test namespace=default
2024-03-05T08:14:53.436139Z  INFO temporal_sdk_core::worker: Activity polling is disabled for this worker
    ✔ should fall back to use getServicesConfig while replaying a non-patched workflow (1242ms)
  1 passing (2s)
Cleaning up source map parsing context

 ——————————————————————————————————————————————————————————————————
 >  NX   Successfully ran target test for project workflows-common-services (16s)

Rerunning the same test case with the fix:

  services
Preloading source map parsing context
2024-03-05T08:15:17.629649Z  INFO temporal_sdk_core: Registering replay worker task_queue="test"
2024-03-05T08:15:17.629699Z  INFO temporal_sdk_core::worker: Initializing worker task_queue=test namespace=default
2024-03-05T08:15:17.629784Z  INFO temporal_sdk_core::worker: Activity polling is disabled for this worker
    ✔ should fall back to use getServicesConfig while replaying a non-patched workflow (1294ms)
  1 passing (2s)

 ——————————————————————————————————————————————————————————————————
 >  NX   Successfully ran target test for project workflows-common-services (13s)

Overall, execution time of the test running has decreased from 16s to 13s by not waiting for the 10s timeout. (There is some other cleanup work that's happening as well, which is preventing the test suite from shutting down even faster after the test has completed. But evern second in lost developer productivity counts. 😄)

Checklist

  1. Closes: none. Didn't file an issue for this because it doesn't affect normal usage of the worker and the fix is trivial.

  2. How was this tested: Tested this locally while executing our own test suite. (See above.)

  3. Any docs updates needed? No.

The `parseWorkflowCode` function creates a 10 second timeout to keep the script and context from getting garbage collected. For normal usage that timeout is not a problem, since workers are expected to run for longer than 10 seconds anyway.

However, when running unit tests these timers can keep the Node.js event loop from shutting down even after the tests have completed successfully, if the tests complete in less than 10 seconds. That's especially common when running individual tests cases rather than the entire test suite.

The fix is rather simple. We just need to "unreference" the timer, to prevent it from keeping the event loop alive unnecessarily.
@jhecking jhecking requested a review from a team as a code owner March 5, 2024 08:37
@mjameswh
Copy link
Contributor

mjameswh commented Mar 5, 2024

Good catch. Makes perfect sense. Thanks!

@CLAassistant
Copy link

CLAassistant commented Mar 5, 2024

CLA assistant check
All committers have signed the CLA.

@mjameswh mjameswh merged commit a16ab49 into temporalio:main Mar 26, 2024
29 of 30 checks passed
@jhecking jhecking deleted the patch-2 branch March 27, 2024 02:08
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

Successfully merging this pull request may close these issues.

3 participants