-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
proc/debuginfod: add timeouts to debuginfod-find #3907
base: master
Are you sure you want to change the base?
Conversation
Calls to debuginfod-find can take an arbitrarily long time to complete. Set timeouts for them unless they were explicitly set by user, also redirect its stderr to our stderr so that, if they produce progress output, it is seen. Updates go-delve#3906
|
||
func execFind(args ...string) (string, error) { | ||
if _, err := exec.LookPath(debuginfodFind); err != nil { | ||
return "", err | ||
} | ||
cmd := exec.Command(debuginfodFind, args...) | ||
if os.Getenv(debuginfodMaxtimeEnv) == "" || os.Getenv(debuginfodTimeoutEnv) == "" { | ||
cmd.Env = append(os.Environ(), debuginfodMaxtimeEnv+"=1", debuginfodTimeoutEnv+"=1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A 1 second timeout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's the minimum possible timeout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but is 1 second reasonable enough in practice? Or will it effectively remove this feature for users (by setting such a low threshold)?
What about adding more output separate from the debuginfod-find response, such as simply notifying the user of what we're doing behind the scenes to make the pause not so confusing. Another option (maybe a future feature) would be to ask the users permission to download any external resource and then upon confirmation we continue, but the user already knows what's happening so there aren't any surprises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could set instead DEBUGINFOD_PROGRESS. The problem is that this is still going to be invisible with other frontends (vscode-go, goland), I think. We are strictly limited to what our interface can look like by the existence of those other frontends, in this case.
One second is enough to get already downloaded symbols, if users need more they can opt-in by setting the environment variables explicitly (maybe we should add those to the documentation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good thought (re: DEBUGINFOD_PROGRESS). For IDE integrations, is there any prior art for this, or standard within DAP for the debug server to send a message to the client to display? There are certain events mentioned in the standard such as the Progress Start/Update/End events, but that might be tedious to implement, as we currently don't expose any of that information at all, so plumbing it back up might not be worth it.
@hyangah (I know you've moved on, but maybe can ping other folks) any thoughts?
Calls to debuginfod-find can take an arbitrarily long time to complete.
Set timeouts for them unless they were explicitly set by user, also
redirect its stderr to our stderr so that, if they produce progress
output, it is seen.
Updates #3906