Add context-aware versions of several functions and some methods - #7007
Add context-aware versions of several functions and some methods#7007nalind wants to merge 3 commits into
Conversation
f089937 to
308bdb1
Compare
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
ack, thanks. I have not looked at all the API changes but I think the context creation is with oncevalue is broken like that
| } | ||
|
|
||
| func getContextWithCancel() (context.Context, context.CancelFunc) { | ||
| return sync.OnceValues(func() (context.Context, context.CancelFunc) { |
There was a problem hiding this comment.
This cannot work can it? Calling sync.OnceValues every single time means you get a new context every single time.
sync.OnceValues must either be assigned to a global variable here
var getContextWithCancel = sync.OnceValues(func() (context.Context, context.CancelFunc) {...
There was a problem hiding this comment.
Oof, yeah, that would've been too easy to keep self-contained. Reworking it.
| os.Setenv("TMPDIR", parse.GetTempDir()) | ||
|
|
||
| if err := rootCmd.Execute(); err != nil { | ||
| err := rootCmd.Execute() |
There was a problem hiding this comment.
FWIW there is ExecuteContext though which sets the ctx field for the cmd though I guess that is not used anywhere so it does not matter really.
There was a problem hiding this comment.
I think trying to pass the context to ExecuteContext() would create an order of initialization problem - the one we initialize is affected by the CLI flags, and they haven't been parsed yet at that point.
There was a problem hiding this comment.
I guess you are right, however having the sync.Once makes it very easy to have this being called anywhere before that as well and then not noticing the flag takes no effect. But I guess that should be possible to catch with tests?
There was a problem hiding this comment.
Yes! It turns out that called for more checking for cancellation at various places, but I added a test for at least one of them.
| defer cancel() | ||
| time.Sleep(1 * time.Millisecond) |
There was a problem hiding this comment.
you can avoid the sleep, just call cancel() without defer which should be enough
b2553e5 to
f48b3fe
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
@Luap99 once this lands and gets vendored I can check whether the prune --build flake in podman#28868 stops showing.. |
941e3a0 to
ac51c68
Compare
|
Rebased and fixed some warnings that the "usestdlibvars" linter would warn about. |
Fixup cases where we were passing a format specifier for the error detail when calling an assert or require helper function, but not using the variant of the helper function that expects a format specifier, and cases where we weren't passing a format specifier, but were using the variant that expects one. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Introduce versions of a number of functions and some methods which take a context.Context, to least try to provide proper cancellation support. When copier.Get() returns an error, also write a non-NUL string after whatever it's already written, so that a tar.Reader that reads what it output will also flag the malformed header with an unexpected error. In main(), connect the global default context to use a timeout set by an experimental timeout flag, and to be canceled on the interrupt signal. Add cancellation checks in a number of functions. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
e702fea to
388f430
Compare
|
Reordered a couple of the commits to trigger CI. |
3 participants
What type of PR is this?
/kind other
What this PR does / why we need it:
Introduce versions of a number of functions and some methods which take a context.Context, to least try to provide proper cancellation support.
When copier.Get() returns an error, also write a non-NUL string after whatever it's already written, so that a tar.Reader that reads what it output will also flag the malformed header with an unexpected error.
In the CLI, pass our default global context down in places where we didn't before, connect it to an experimental timeout flag, and have it cancel on
os.Interruptsignals, too.How to verify it
New/updated unit tests!
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Does this PR introduce a user-facing change?