Add cancellable attach. #432
Conversation
|
I've extended this to StartExec now. What do you think? Would you prefer we pass in a channel the user can close instead? |
|
@fsouza ping? |
|
Hi @tomwilkie, thank you very much for working on that! I'm planning to have a look at it later today or tomorrow, in the worst case. I've seen your comment on #161, the idea looks promising. Sorry for taking that long to get back to you, and thank you again for contributing! |
|
Not a problem! Let me know which way you prefer (returning an WaitCloser vs passing a channel) and I'll code it up. |
|
@tomwilkie I do prefer the WaitCloser approach, looking at the code at this moment! |
| return err | ||
| } | ||
| return cw.Wait() | ||
| } |
There was a problem hiding this comment.
Maybe AttachToContainer could just call AttachToContainerNonBlocking and then wait on the CloseWaiter? It would avoid code duplication.
The same applies for StartExec.
There was a problem hiding this comment.
Yeah, good idea. Will do.
|
I liked your implementation, I was wondering whether we could make CloseWaiter a parameter in AttachToContainerOptions (and StartExecOptions), along with a boolean flag for identifiying whether we want to block or not, but maybe just having another method is a better idea! May I invoke others here? @peter-edge @cezarsa @andrewsmedina what do you all think? :-) |
Thanks!
We could add it as a "out" parameter in the struct I guess - and that would solve the issue of naming the second copy of the function. Let me have a look and see how it feels - my initial thought is having the method in question modify the struct being passed in (and/or change its basic behaviour based on a bool) isn't particularly desirable. |
Oh yes please! |
| if err != nil { | ||
| return err | ||
| } | ||
| if cw != nil { |
There was a problem hiding this comment.
Is there a scenario where both cw and err are nil?
There was a problem hiding this comment.
Sorry, just checked and there is hahaha
Sorry about that.
|
I've spoken with @andrewsmedina and @cezarsa offline and we believe that it's good to merge. Thanks for contributing! |
|
Thanks! |
|
Just saw this one fly by. A bit unfortunate it doesn't use x/net/context. Is it straightforward to build context-based cancellation on top of this? |
|
@pwaller yes it should be relatively straight forward to hook this up to context-based cancellation. |
Let me know what you think to the approach - I can extend it to exec also.
Fixes #161