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

Add syscallWatcher for some missing syscalls #388

Closed
wants to merge 1 commit into from

Conversation

jterry75
Copy link
Contributor

Signed-off-by: Justin Terry (VM) juterry@microsoft.com

Signed-off-by: Justin Terry (VM) <juterry@microsoft.com>
@lowenna
Copy link
Contributor

lowenna commented Nov 26, 2018

It was deliberate to not put these around (especially) Resize as Docker CLI triggers this IIRC every 200ms, during an interactive session due to lack of equivalence to SIGWINCH. This will end up building a huge number of goroutines in any caller, so I would definitely not include this one. Perhaps a comment as to why would be relevant. To the best of my knowledge, I don't think I've seen any of the others ever hang, perhaps with the exception of CloseStdin.

@jstarks
Copy link
Member

jstarks commented Nov 26, 2018

On a side note, I believe in recent versions of Windows 10, the console does send window size change events on request. So we could probably fix the Docker client at this point.

@jterry75
Copy link
Contributor Author

I can remove it from Resize but it seems to me that this could hang anywhere so why would we special case any syscalls?

@jterry75
Copy link
Contributor Author

jterry75 commented Nov 28, 2018

@jhowardmsft - Its your call here. I can close this PR if we think none of these are helpful or if they are too verbose. Seems like your concern could also be alleviated by a context.Context cancellation once a syscall completes which would stop the syscallWatcher goroutine and thus the thread only lives in the lifetime of a single hang or a single completion.

@lowenna
Copy link
Contributor

lowenna commented Nov 29, 2018

I'd prefer to close this PR for now, until something like your suggestion of a cancellation could be implemented (which is a really good thing to add regardless)

@jterry75
Copy link
Contributor Author

Ok np

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants