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

fix: fuels dev cleanup not killing node #3038

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

nedsalk
Copy link
Contributor

@nedsalk nedsalk commented Aug 27, 2024

Summary

Checklist

  • All changes are covered by tests (or not applicable)
  • All changes are documented (or not applicable)
  • I reviewed the entire PR myself (preferably, on GH UI)
  • I described all Breaking Changes (or there's none)

@nedsalk nedsalk added the bug Issue is a bug label Aug 27, 2024
@nedsalk nedsalk self-assigned this Aug 27, 2024
Copy link

vercel bot commented Aug 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
create-fuels-template ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 18, 2024 9:32am
fuels-template ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 18, 2024 9:32am
ts-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 18, 2024 9:32am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
create-fuels-counter-example ⬜️ Ignored (Inspect) Nov 18, 2024 9:32am
create-fuels-template-preview ⬜️ Ignored (Inspect) Nov 18, 2024 9:32am
ts-docs-api ⬜️ Ignored (Inspect) Nov 18, 2024 9:32am

@@ -233,7 +233,6 @@ export const launchNode = async ({
}
childState.isDead = true;

removeSideffects();
Copy link
Contributor Author

@nedsalk nedsalk Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this line caused the issue to go away. I suspect that the root cause of the problem is somewhere in the fuels package because this line didn't cause problems to e.g. launchTestNode. It might be in the interplay between file watchers registered in the dev command and this cleanup function removing the files they're watching, but I couldn't pinpoint the actual root cause. Tried deleting lines I think might be causing it while having this line still on, but to no avail. We can search for the root cause in another issue that's of lesser priority.
The cleanup still happens because of the exit and error event listeners registered on the child above.

it(
'cleans up resources on graceful shutdown',
async () => {
using paths = runInit();
Copy link
Contributor Author

@nedsalk nedsalk Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The using keyword is great because you don't have to think about where the test might fail so that you run the cleanup if it does fail there. With using, the cleanup will happen automatically both when the test finishes as well as when it throws an error.

Copy link

codspeed-hq bot commented Nov 15, 2024

CodSpeed Performance Report

Merging #3038 will degrade performances by 54.32%

Comparing ns/fix/fuels-dev-node-cleanup (4227cc9) with master (537fdc2)

Summary

❌ 1 regressions
✅ 17 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master ns/fix/fuels-dev-node-cleanup Change
should successfully transfer a single asset between wallets (x20 times) 61.7 ms 135 ms -54.32%

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

Successfully merging this pull request may close these issues.

Exiting fuels dev with CTRL+C (SIGINT) doesn't kill fuel-core node
1 participant