Skip to content
This repository has been archived by the owner on Sep 14, 2023. It is now read-only.

fix: process resource cleanup in start task #648

Merged
merged 14 commits into from
Feb 24, 2023

Conversation

kratico
Copy link
Contributor

@kratico kratico commented Feb 23, 2023

Based on trial and error, it seems that sometimes zombienet Deno resources are not properly released.
I haven't been able to identify the root cause.
The unload event is not fired in these cases.

self.addEventListener("unload", () => Deno.exit(status.code))

This PR

  • closes child processes started with Deno.run
  • closes child processes piped file descriptors
  • upgrades polkadot, polkadot-parachain and zombienet versions

Note: I've observed that the star task sometimes keeps running even after closing fd and processes

Another theory to explore is that this could be correlated to a recent Deno version upgrade.

This is related to #625

@kratico kratico force-pushed the fix/star-task-resource-cleanup branch from 1e11cac to 45c0e9e Compare February 23, 2023 17:27
@@ -96,7 +96,10 @@ export class ZombienetProvider extends FrameProxyProvider {
})
let closeProcess = () => {
closeProcess = () => {}
process.kill()
console.log("closing zombienet rid:", process.rid)
process.kill("SIGINT")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kratico kratico marked this pull request as ready for review February 23, 2023 18:20
Copy link
Contributor

@harrysolovay harrysolovay left a comment

Choose a reason for hiding this comment

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

LGTM

@kratico kratico added this pull request to the merge queue Feb 23, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 23, 2023
@harrysolovay harrysolovay added this pull request to the merge queue Feb 23, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 23, 2023
Copy link
Contributor

@harrysolovay harrysolovay left a comment

Choose a reason for hiding this comment

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

Let's actually hold off on merging this, as we will likely want to capture dev process std out for signaling boot progress #649

@kratico
Copy link
Contributor Author

kratico commented Feb 24, 2023

Let's actually hold off on merging this, as we will likely want to capture dev process std out for signaling boot progress #649

@harrysolovay
Do you mean something like await copy(process.stderr, Deno.stderr) for the PolkadotDevProvider?

Would it be for boot progress or error debugging?

@harrysolovay
Copy link
Contributor

Both. I suppose we can tackle that in a subsequent PR. This looks good.

@harrysolovay harrysolovay added this pull request to the merge queue Feb 24, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 24, 2023
@kratico kratico added this pull request to the merge queue Feb 24, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 24, 2023
@kratico kratico added this pull request to the merge queue Feb 24, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 24, 2023
@kratico kratico added this pull request to the merge queue Feb 24, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 24, 2023
@harrysolovay harrysolovay added this pull request to the merge queue Feb 24, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 24, 2023
@tjjfvi tjjfvi added this pull request to the merge queue Feb 24, 2023
@tjjfvi tjjfvi removed this pull request from the merge queue due to a manual request Feb 24, 2023
@harrysolovay harrysolovay force-pushed the fix/star-task-resource-cleanup branch from 045c683 to 4698e78 Compare February 24, 2023 15:08
@harrysolovay harrysolovay merged commit b3102c6 into main Feb 24, 2023
@harrysolovay harrysolovay deleted the fix/star-task-resource-cleanup branch February 24, 2023 15:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants