Add command-line argument --timeout, to configure network timeout values

[amotl]

About

Coming from GH-370, this patch introduces the --timeout command-line argument to specify the (connect, read) timeout values. The default connect timeout is five seconds now, the default read timeout is infinite.

Backlog

• Needs Permit urllib3.Timeout instances for defining timeout values crate-python#603.

[matriv]

thx, left some comments/questions

[matriv]

just double checking, infinite is a valid value that is parsed by urllib3.Timout(), right?

[amotl]

Thanks for asking. "infinite" isn't a valid value, it is just a surrogate. When obtaining Python's None symbol, the timeout will be infinite. Currently, there is no way to obtain this value from a text representation from the command line.

[amotl]

Do you have a suggestion how to improve code or docs around this topic, either by actually implementing inf(inite) as a surrogate keyword representing None, or, alternatively, by improving the documentation / help / guidance texts?

[surister]

I'd say None is fine, I like it, since None does seem to be specially valuable here, and it actually reads nice as ìf timeout is None -> no timeout, hence 'infinite'.

Otherwise sentinel object but imo it's too much

[amotl]

Yeah, I also think it is fine from the perspective of the Python API, and I agree that a sentinel object is not needed as a surrogate for None here.

In this case, I think it would be more about discussing the need to find a corresponding textual representation to make the "infinite" surrogate be obtainable from the command line via --timeout=. Would you prefer that the literal "None" would be translated to None, or would you prefer that the literal "inf" or "infinite" would be translated to None?

Without implementing that, I think there would currently be no possibility to configure, for example, an infinite connect timeout. We would need to check / validate what --timeout=,42 does, and eventually shape it a bit more to converge this merely undefined behaviour into something with a more sensible semantic.

[matriv]

imho, we should give the chance to the user to configure the default "infinite" value, if needed,
so I'd go with None (case insensitive though, so that none, NONE, NoNE, etc. is all the same)

[matriv]

We could also use 0 or -1 on the cmd line to make it translate internally to infinite.

[mfussenegger]

Good call. 0 or -1 are commonly used to set no-timeout/infinite. +1

[amotl]

Thanks. The patch has been amended to implement your suggestion. Now, -1 can be used as a surrogate value to represent None, which means "infinite timeout".

[amotl]

The patch has been significantly amended, so I am humbly asking for another review. Thank you again.

[mfussenegger]

Suggested change

	raise ValueError(f"Decoding timeout values failed: {timeout}")
	raise ValueError(f"Cannot decode timeout {timeout}, expected format `<connect_sec>,<read_sec>`")

[amotl]

Thanks. I've adjust this error message, and also aligned the other one for the TypeError a few lines above.