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

Tests fail at high build concurrency for default openssh config #135

Open
FauxFaux opened this issue Oct 7, 2019 · 2 comments

Comments

@FauxFaux
Copy link
Contributor

commented Oct 7, 2019

cargo test fails for me most of the time with --test-threads=24, even on a dual core machine, and ~30% of the time with $(nproc) == 12.

Random tests fail with: 'called Result::unwrap() on an Err value: Error { code: -43, msg: "Failed getting banner" }', at

sess.handshake().unwrap();

-43 is LIBSSH2_ERROR_SOCKET_RECV.

If you enable debugging (cfg.define("LIBSSH2DEBUG", None); cfg.define("HAVE_GETTIMEOFDAY", None); and cripple the filtering) you can see that the connection is being closed.

Initially I thought this was a horrible thread safety bug (and spent ages on that), but it's probably just openssh's default MaxSessions 10, which means no more than 10 people can be logging in at a time, so, uh, probably a minor documentation note, instead of the cool bug I was hoping for?

@wez

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2019

yeah, that's frustrating. I spent some time troubleshooting failures in the CI and cooked up this script. What seemed to be the critical thing there was the MaxStartups value:

https://github.com/alexcrichton/ssh2-rs/blob/master/tests/run_integration_tests.sh#L51

In terms of making things more discoverable for cargo test, what do you think about having run_integration_tests.sh export an env var to indicate that it is being used to run the tests, and then having the tests check for that and emitting a message to tell the user to run the script instead of cargo test directly?

@FauxFaux

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

That's a nice idea.

Much better than my proposal (documentation on github!), or trying to teach the tests to limit concurrency themselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.