Skip to content

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

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
Closed
mafredri opened this issue Sep 27, 2024 · 2 comments · Fixed by #368 or #375
Closed

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 or #375
Assignees
@mafredri

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 <[email protected]>
(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.

@mafredri mafredri assigned mafredri and unassigned johnstcn Sep 30, 2024
@mafredri mafredri mentioned this issue Sep 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mafredri mafredri

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@mafredri @johnstcn