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

chore: Remove CommandContext.CommandContext #410

Merged
merged 2 commits into from
Oct 21, 2024

Conversation

bgins
Copy link
Contributor

@bgins bgins commented Oct 21, 2024

Summary

This pull request makes the following changes:

  • Remove CommandContext.CommandContext

We set up the command context with a CommandContext.CommandContext and a CommandContext.Ctx, but they are the same context:

ctx, cancel := signal.NotifyContext(ctx, os.Interrupt)
return &CommandContext{
CommandContext: ctx,
Ctx: ctx,
Cm: cm,
CancelFunc: cancel,
}

CommandContext.CommandContext is not referenced anywhere outside the CommandContext type, the set up function, and a clean up call. It seems safe to remove.

Task/Issue reference

Closes: https://github.com/Lilypad-Tech/internal/issues/296

Test plan

Run a job. The job should complete successfully.

When services are shut down after the job, they should no longer emit a CleanupManager: Cleanup called again after already called warning.

Details

We initially noticed this issue when we started seeing CleanupManager: Cleanup called again after already called warnings. Cleanup was called on (effectively) the same context, and the same set of functions.

The cleanup manager design isn't entirely clear. We think that functions could be associated with their contexts, and then each cleanup function could be run with its context. The current design does not support this approach.

We may want to revisit in the future if we decide we would like more than one context.

@bgins bgins requested a review from a team as a code owner October 21, 2024 19:01
@cla-bot cla-bot bot added the cla-signed label Oct 21, 2024
@github-actions github-actions bot added the chore label Oct 21, 2024
Copy link
Collaborator

@walkah walkah left a comment

Choose a reason for hiding this comment

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

Can we just remove CommandContext.CommandContext altogether? It looks to me like it's never used. (remove it from the strct and in NewSystemContext just don't set it).

But otherwise, removes the double cleanup warning so 👍 🙏

@walkah
Copy link
Collaborator

walkah commented Oct 21, 2024

This context is unused and the same context as CommandContext.Ctx
@bgins
Copy link
Contributor Author

bgins commented Oct 21, 2024

Can we just remove CommandContext.CommandContext altogether? It looks to me like it's never used. (remove it from the strct and in NewSystemContext just don't set it).

Yeah, good call! Removed it and updated the top-level PR description.

@bgins bgins changed the title chore: Remove redundant cleanup call chore: Remove CommandContext.CommandContext Oct 21, 2024
@bgins bgins merged commit 22a310c into main Oct 21, 2024
8 checks passed
@bgins bgins deleted the bgins/chore-remove-redundant-cleanup-call branch October 21, 2024 21:13
@bgins bgins self-assigned this Dec 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants