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

Jump to bottom

x/crypto/ssh: performance issues after #24942 was fixed #32034

Closed
davidrenne opened this issue May 14, 2019 · 4 comments
Closed

x/crypto/ssh: performance issues after #24942 was fixed #32034

davidrenne opened this issue May 14, 2019 · 4 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@davidrenne
Copy link

Hi there, I am not sure what @ianlancetaylor or @mikioh and other maintainers of golang would do about reverting this commit:

60e3ebb

And this discussion:

#24942 (comment)

But I just wanted to express what I have been seeing happening with this change affecting performance on an SSH connection.

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

$ go version 1.11

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env

GOARCH="amd64"

GOBIN=""

GOCACHE="/Users/davidrenne/Library/Caches/go-build"

GOEXE=""

GOHOSTARCH="amd64"

GOHOSTOS="darwin"

GOOS="darwin"

GOPATH="/Users/davidrenne/Atlona-Dev"

GORACE=""

GOROOT="/usr/local/go"

GOTMPDIR=""

GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"

GCCGO="gccgo"

CC="clang"

CXX="clang++"

CGO_ENABLED="1"

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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/0_/nswys1md33gflzj54n8r415r0000gn/T/go-build038885308=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

#24942 and this commit

60e3ebb

Has caused extreme slowness in an SSH connection we are making to a CISCO video codec device using this library to make the connection:

https://gist.github.com/davidrenne/1254918151e1c8eda592928f1b72b5d5

On line 162:

sshConn, chans, reqs, err := ssh.NewClientConn(conn, address, config)

This takes an extreme amount of time to return for some reason and hangs because of this commit @fraenkel added to set it to non blocking.

We have been upgrading golang since June 2016 in our project which maintains network connections to SSH, Telnet, TCP, Websocket, UDP and more, but now we are stopped in our tracks a little bit with the upgrade path and seemingly stuck on 1.10.8. In that build, our customers will experience quick connections to SSH to this CISCO device in question.

I have build golang from source to find which commit is causing this issue for us and for sure when I go to 68c1028 (syscall: avoid extra syscall on send/recvmsg on Linux) we do not see any issues in connecting slowly. The SSH connection is made quickly, but when I go to one commit ahead to 60e3ebb, its back to being unacceptably slow.

Here is the link in the tree where this commit happened to cause some issues for our product:

https://github.com/golang/go/commits/release-branch.go1.11?before=efa061d9f5d52846dfc3dda40eaf8eccfeeae8d2+1365

What did you expect to see?

SSH connection performance like 1.10.8.

What did you see instead?

Slowness taking several minutes (for this device I am triaging at least) for the line of code

sshConn, chans, reqs, err := ssh.NewClientConn(conn, address, config)

To return back to the caller.
@bradfitz bradfitz changed the title golang.org/x/crypto/ssh performance issues after #24942 was fixed x/crypto/ssh: performance issues after #24942 was fixed May 14, 2019
@gopherbot gopherbot added this to the Unreleased milestone May 14, 2019
@bradfitz
Copy link
Contributor

/cc @hanwen

@hanwen
Copy link
Contributor

hanwen commented May 14, 2019

this looks like an issue in the networking code. You could run with debugHandshake to see what is going on .

I would also suggest to remove the keepalive stuff. SSH has its own keepalive mechanism which would be preferable. Just do like here:

https://go.googlesource.com/crypto/+/df01cb2cc480549d72034218dd98bf97671450ac/ssh/session_test.go#578

@davidrenne
Copy link
Author

OMG @hanwen thanks for such a quick turn around, I will play around a bit. Thank you!

@davidrenne
Copy link
Author

@hanwen yes it seems that the TCP keep alive we were using was the culprit for some reason hanging up on the SSH side, when we dont see the same issue on other TCP connections. I am able to connect quickly on 1.12.5 now with the keepalive code taken out.

I also added your keepalive in its place and hoping it keeps the connections open for most devices

		if _, err := session.SendRequest("keepalive@openssh.com", true, nil); err != nil {
			if terminal.OnError != nil {
				terminal.OnError(terminal, err, "Failed to keepalive")
				conn.Close()
			}
			time.Sleep(time.Millisecond * time.Duration(RECONNECTION_TIME))
			continue
		}

Thank you very much for your help @hanwen, you did your good deed for the day! Now go get a beer on me! ;-)

I wish I just understood why that non-blocking commit conflicted with the github.com/felixge/tcpkeepalive package on SSH. A lot of this stuff is new territory for me personally. I did set debugHandshake and I will use that in the future for any SSH related packet issues, that is a very helpful logging flag to set when you have to debug a connection handshake.

@golang golang locked and limited conversation to collaborators May 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@bradfitz @hanwen @davidrenne @gopherbot