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.

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

proc/debuginfod: add timeouts to debuginfod-find #3907

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

proc/debuginfod: add timeouts to debuginfod-find #3907

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion pkg/proc/debuginfod/debuginfod.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,26 @@
package debuginfod

import (
"os"
"os/exec"
"strings"
)

const debuginfodFind = "debuginfod-find"
const (
debuginfodFind = "debuginfod-find"
debuginfodMaxtimeEnv = "DEBUGINFOD_MAXTIME"
debuginfodTimeoutEnv = "DEBUGINFOD_TIMEOUT"
)

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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A 1 second timeout?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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).

Copy link
Member

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?

}
cmd.Stderr = os.Stderr
out, err := cmd.Output() // ignore stderr
if err != nil {
return "", err
Expand Down