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

Jump to bottom

cmd/pprof: Control-C does not exit interactive mode #27147

Closed
mvdan opened this issue Aug 22, 2018 · 11 comments
Closed

cmd/pprof: Control-C does not exit interactive mode #27147

mvdan opened this issue Aug 22, 2018 · 11 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@mvdan
Copy link
Member

mvdan commented Aug 22, 2018

What version of Go are you using (go version)?

go version devel +f4e4ec2cd0 Wed Aug 22 06:54:03 2018 +0000 linux/amd64

Does this issue reproduce with the latest release?

No - works fine on go version go1.10.3 linux/amd64.

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/mvdan/go/cache"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/mvdan/go/land:/home/mvdan/go"
GOPROXY=""
GORACE=""
GOROOT="/home/mvdan/tip"
GOTMPDIR=""
GOTOOLDIR="/home/mvdan/tip/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build845884737=/tmp/go-build -gno-record-gcc-switches"

What did you do?

$ go tool pprof cpu.out
File: json.test
Type: cpu
Time: Aug 22, 2018 at 1:52pm (BST)
Duration: 7.05s, Total samples = 20.59s (292.15%)
Entering interactive mode (type "help" for commands, "o" for options)
(pprof) # hit control-C

What did you expect to see?

The same that I see via Go 1.10:

$ go1 tool pprof cpu.out
File: json.test
Type: cpu
Time: Aug 22, 2018 at 1:52pm (BST)
Duration: 7.05s, Total samples = 20.59s (292.15%)
Entering interactive mode (type "help" for commands, "o" for options)
(pprof) ^C
$

What did you see instead?

^C having no effect whatsoever. The only way to exit is now to type exit, or ^D (EOF).

I presume this is a regression in 1.11, likely because of all the readline changes that were retrofitted into go tool pprof. cc @hyangah

Milestoning for 1.12 for now, but this is a regression in a way.
@mvdan mvdan added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 22, 2018
@mvdan mvdan added this to the Go1.12 milestone Aug 22, 2018
@mvdan
Copy link
Member Author

mvdan commented Aug 22, 2018

Another weird behavior is that pressing ^D only has an effect if there's an empty command. That is, writing top^D doesn't actually run the top command either. Although this doesn't work on 1.10 either.

@hyangah
Copy link
Contributor

hyangah commented Sep 4, 2018

Yes, that's the regression due to the readline changes.
I tried to replace the terminal package with github.com/chzyer/readline during 1.11
cycle in order to address this regression, but it was late in the dev cycle and chzyer/readline
had a few other issues to be addressed so I just gave up.

I think it's desirable for the upstream pprof and the go tool pprof to to go with the
same readline package. But I don't know if that will be the current terminal package or
other 3rd party readline packages. Anyway, it should be decided before Go1.12.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Go1.13 Dec 10, 2018
@mvdan
Copy link
Member Author

mvdan commented Jan 4, 2019

We obviously missed this for 1.12 :) These little readline issues bite me at least once a week - I'd almost give up the possibility to walk up the history to just fix these bugs. So I do think we should fix it for 1.13.

Remind me - what's the issue with golang.org/x/crypto/ssh/terminal? I find it a bit weird that the package is under crypto/ssh, but otherwise it's a terminal package, so shouldn't we just fix these bugs there?

Also, I assume that the reason we don't want a third-party terminal package inside Go is bloat. Why not go the other way, and have the third-party pprof tool use the same readline library as the one in cmd/pprof?

@hyangah
Copy link
Contributor

hyangah commented Jan 4, 2019 •
edited

Update: I gave up my original plan of using the third-party terminal package. Sent a PR to upstream to be compatible with our builders and didn't get responses. I now think depending on third-party packages that we can't fix quickly doesn't seem like a good idea.

Let's fix golang.org/x/crypto/ssh/terminal. My concern on this package was, it's minimal and has been a less popular choice among go users - meaning less chances of being tested. But at least, it allows us to maintain the source and fix bugs promptly.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@hyangah hyangah mentioned this issue Mar 6, 2020
Closed
@hyangah
Copy link
Contributor

hyangah commented Mar 6, 2020

Sorry for the delay.
I believe we have now a better story about importing 3rd party dependencies, so I am proposing to retry to switch to github.com/chzyer/readline like the upstream.

@gopherbot
Copy link

Change https://golang.org/cl/222342 mentions this issue: cmd/pprof: use github.com/chzyer/readline for readline support

@hyangah
Copy link
Contributor

hyangah commented Mar 6, 2020 •
edited

The situation didn't change much - go builders do not like the new dependency, and we need help from github.com/chzyer/readline maintainers to address the issues or accept PRs.

@gopherbot
Copy link

Change https://golang.org/cl/228223 mentions this issue: ssh/terminal: handle ctrl+C, ctrl+F, ctrl+B

gopherbot pushed a commit to golang/crypto that referenced this issue Apr 14, 2020
@hyangah
ssh/terminal: handle ctrl+C, ctrl+F, ctrl+B
4f8f47a 
ctrl+C: terminate readline, which may result in application termination.
ctrl+F: keyRight
ctrl+B: keyLeft

Update golang/go#27147

Change-Id: If319ef79708b98c030cbce102400a785d15137f8
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/228223
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/228226 mentions this issue: cmd: update golang.org/x/crypto@4f8f47a

@hyangah
Copy link
Contributor

hyangah commented Apr 14, 2020

Obviously I gave up my plan to switch the readline dependency and addressed the issue in the golang.org/x/crypto/ssh/terminal package.

I am not sure about the behavior of 'Ctrl+D' mentioned in #27147 (comment) yet
(Currently when the line is not empty and 'Ctrl+D' is detected, the terminal package interprets it as a deletion, which I think is correct) If this is a problem, let's follow up in a separate issue.

@mvdan
Copy link
Member Author

mvdan commented Apr 14, 2020

If ^C works, I don't mind the ^D behavior as much. I was only trying it as an alternative to exit the prompt when there's a line written.

gopherbot pushed a commit to golang/term that referenced this issue Oct 16, 2020
@hyangah
ssh/terminal: handle ctrl+C, ctrl+F, ctrl+B
07bee37 
ctrl+C: terminate readline, which may result in application termination.
ctrl+F: keyRight
ctrl+B: keyLeft

Update golang/go#27147

Change-Id: If319ef79708b98c030cbce102400a785d15137f8
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/228223
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Apr 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@rsc @andybons @ianlancetaylor @mvdan @hyangah @gopherbot