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.

Sign up for GitHub

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

Jump to bottom

Defers run in cmd/envbuilder won't execute because Run performs execve(2) #364

Open
mafredri opened this issue Sep 27, 2024 · 2 comments · Fixed by #368
Open

Defers run in cmd/envbuilder won't execute because Run performs execve(2) #364

mafredri opened this issue Sep 27, 2024 · 2 comments · Fixed by #368
Assignees
@johnstcn

Comments

@mafredri
Copy link
Member 

          Review: Even though we've fixed defers here, there's still `cmd/envbuilder` where e.g. `closeLogs` won't fire since we execve(2) after this.

Originally posted by @mafredri in #359 (comment)
@mafredri mafredri mentioned this issue Sep 27, 2024
Merged
@johnstcn
Copy link
Member

johnstcn commented Sep 27, 2024 •
edited
Loading

We could maybe add a PreExec []func() field to envbuilder.Options that will get called right before we Exec?

Edit: it's simpler to just pass them into Run() / RunCacheProbe.

@johnstcn johnstcn self-assigned this Sep 27, 2024
@johnstcn johnstcn mentioned this issue Sep 27, 2024
Merged
johnstcn added a commit that referenced this issue Sep 27, 2024
@johnstcn
fix: add preExec to Run (#368)
9ee20e7 
Fixes #364

Adds the capability for Run() to run defers defined outside of Run().
Currently the only use-case for this is to close the Coder logger.
Also adds an integration test that validates this behaviour and ensures that closeLogs gets called.

Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
(cherry picked from commit 481e3a9)
@johnstcn johnstcn reopened this Sep 27, 2024
@johnstcn
Copy link
Member

Need to be careful about LogSenderV2 (see #371); will fix this later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@johnstcn johnstcn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: add preExec to Run coder/envbuilder
2 participants
@mafredri @johnstcn