Home Original page

bpo-29970: Make ssh_handshake_timeout None by default by asvetlov · Pull Request #4939 · python/cpython

@asvetlov

Float default prevents checking for passing ssh_handshake_timeout with plain socket connection.

Raise ValueError if ssh_handshake_timeout is specified without ssl.

Postfix for #4825

https://bugs.python.org/issue29970

@asvetlov 

Add a check for passing the parameter only with ssl context.

@asvetlov asvetlov changed the title Make ssh_handshake_timeout None by default. bpo-29970: Make ssh_handshake_timeout None by default

Dec 20, 2017

@asvetlov

1st1
if ssl_handshake_timeout is not None and not ssl:
raise ValueError(
'ssl_handshake_timeout is only meaningfu with ssl')

Choose a reason for hiding this comment

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

meaningfu -> meaningful

Choose a reason for hiding this comment

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

Done

@1st1

BTW, what happens if someone passes ssl_handshake_timeout=0.0? Do we need to explicitly prohibit this?

@asvetlov

@asvetlov

zero and negative timeout is an error but I doubt if we should prohibit it: there still is a possibility to pass 1e-30 which is obviously wrong.

@1st1

zero and negative timeout is an error but I doubt if we should prohibit it: there still is a possibility to pass 1e-30 which is obviously wrong.

In many APIs, passing 0.0 disables the mechanism, and that's why we should either disable the timeout, or raise an error. Otherwise it's just confusing.

@1st1

I vote for raising an error for 0.0. There's no point in allowing to disable the timeout.

If someone passes 1e-10, it's them clearly wanting to get a TimeoutError, that's it.

@asvetlov

Ok, will add a code for raising

@asvetlov

@asvetlov

1st1

1st1 approved these changes Dec 20, 2017

1st1

sslcontext = test_utils.dummy_ssl_context()
app_proto = mock.Mock()
waiter = mock.Mock()
with self.assertRaises(ValueError):

Choose a reason for hiding this comment

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

assertRaisesRegex

@1st1

Wait, please use assertRaisesRegex to validate part of the error message. ValueError can be raised from anywhere, and sometimes assertRaises tests something entirely different.