Skip to content

Conversation

@tdstein
Copy link
Collaborator

@tdstein tdstein commented Feb 8, 2023

Adds support configurable HTTP and HTTPS request timeouts via the environment variable CONNECT_REQUEST_TIMEOUT.

Description

The current implementation uses hardcoded timeout values in different locations throughout the application. Typically, these pre-set timeouts are good enough. But, some users have requested the ability to configure timeouts to address unique constraints in their runtime environment.

This change removes all hardcoded timeouts, and replaces them with calls to rsconnect.timeouts.get_timeout. In turn, this method retrieves the value from the environment variable CONNECT_REQUEST_TIMEOUT. If this variable is unset, a default value of 300 (i.e. 5 minutes) is used.

Motivation and Context

Address Issue #346

Users have requested the ability to configure timeouts when running within a slow network.

This change does not add a corresponding CLI option. Doing so would greatly increase the scope of this pull request since the callstack from rsconnect.main to rsconnect.http_support._connection_factory is deep and the calls are numerous.

If a CLI option is still needed, I would prefer to implement this change in a seperate pull request.

How Has This Been Tested?

Unit tests have been added to validate rsconnect.timeouts.get_timeout.

In addition, a DEBUG message has been added showing what the current timeout is set to for each HTTPConnect and HTTPSConnection.

Screenshots (if appropriate):

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

This change alters the default values for timeouts, resulting in slightly different behavior for users. In most cases, this change will not be noticeable since the new default is greater than all previous defaults.

Checklist:

  • I have added tests to cover my changes.
  • I have updated CHANGELOG.md to cover notable changes.

@tdstein tdstein linked an issue Feb 8, 2023 that may be closed by this pull request
@tdstein tdstein force-pushed the 346-add-timeout-configuration branch 2 times, most recently from 49b4728 to 54bb90b Compare February 8, 2023 20:49
@tdstein tdstein requested review from bcwu and mmarchetti February 8, 2023 20:50
@tdstein
Copy link
Collaborator Author

tdstein commented Feb 8, 2023

@mmarchetti and @bcwu - I've kept this pull request as a draft, but I would appreciate your initial thoughts on this approach. I have outlined my decision-making process in the pull request description.

In particular, is not adding CLI support viable? And is a single global timeout option appropriate? Are there situations where multiple timeouts are needed for a single command?

Also, in case you missed it, here is a relevant discussion from Slack: https://positpbc.slack.com/archives/C021090K8Q5/p1675785843875829

@tdstein tdstein force-pushed the 346-add-timeout-configuration branch from 54bb90b to 445676b Compare February 9, 2023 18:08
@tdstein tdstein force-pushed the 346-add-timeout-configuration branch 2 times, most recently from 8254bc0 to 2d34536 Compare February 9, 2023 18:25
Adds support for configurable HTTP and HTTPS timeouts via the
environment variable "CONNECT_REQUEST_TIMEOUT". When not set, a default
value of 300 seconds is used.
@tdstein tdstein force-pushed the 346-add-timeout-configuration branch from 2d34536 to 6d436f3 Compare February 9, 2023 18:29
@tdstein tdstein marked this pull request as ready for review February 9, 2023 18:40
@bcwu
Copy link
Contributor

bcwu commented Feb 10, 2023

I am supportive of a follow-up PR to add a CLI option. CLI is the most frequent use case. Having a CLI user setting an env var separately instead of having the option organically as part of their CLI workflow would be jarring.

We probably also need an API option for users who use actions.py to publish to Connect.

@tdstein tdstein force-pushed the 346-add-timeout-configuration branch from e778500 to f1054aa Compare February 10, 2023 15:14
@tdstein tdstein force-pushed the 346-add-timeout-configuration branch from 7b30316 to 5c47917 Compare March 13, 2023 15:16
@kgartland-rstudio
Copy link
Contributor

kgartland-rstudio commented Mar 22, 2023

Testing against the latest commit:
rsconnect-python version: 1.15.0b3.dev21+g5c47917

I can't seem to get this to work. I'm testing a Jupiter notebook with a long running cell by including a sleep(310).

Deploy command:

rsconnect deploy notebook --environment CONNECT_REQUEST_TIMEOUT=10 --environment DEBUG=true ./stock-report-jupyter.ipynb ./quandl-wiki-tsla.json.gz -n yeti-password -N -t 'extra long delay'

I would expect this to timeout after 10 seconds. It does not, it deploys and renders properly in Connect. In the Connect application logs I see:

--ExecutePreprocessor.timeout=None

Additionally I do not see any additional debugging info in the Connect application logs. Do I need to test this against a specific Connect branch?

@tdstein
Copy link
Collaborator Author

tdstein commented Mar 22, 2023

Testing against the latest commit: rsconnect-python version: 1.15.0b3.dev21+g5c47917

I can't seem to get this to work. I'm testing a Jupiter notebook with a long running cell by including a sleep(310).

Deploy command:

rsconnect deploy notebook --environment CONNECT_REQUEST_TIMEOUT=10 --environment DEBUG=true ./stock-report-jupyter.ipynb ./quandl-wiki-tsla.json.gz -n yeti-password -N -t 'extra long delay'

I would expect this to timeout after 10 seconds. It does not, it deploys and renders properly in Connect. In the Connect application logs I see:

--ExecutePreprocessor.timeout=None

Additionally I do not see any additional debugging info in the Connect application logs. Do I need to test this against a specific Connect branch?

Hey @kgartland-rstudio , apologies if this wasn't clear. This change sets a timeout on the rsconnect-python internal HTTPClient. It has no effect on the Connect server.

Instead of setting the --enviornment flag, instead set the shells environment variable. And set the -v flag. Setting the verbose flag will log a DEBUG message which logs the following message: https://github.com/rstudio/rsconnect-python/pull/349/files#diff-9bd33200ca8ebda3645a5652ce33334d61e6b7cc5662c23ee35683b1d5915708R81

CONNECT_REQUEST_TIMEOUT=10 rsconnect deploy notebook ./stock-report-jupyter.ipynb ./quandl-wiki-tsla.json.gz -n yeti-password -N -t -v 'extra long delay'

I'm not entirely sure how the rsconnect deploy notebook command interacts with the Connect server, but if the server doesn't respond until the Notebook renders, then the timeout should be hit.

Otherwise, you may have to get creative. One idea I have is setting up macOS "Network Link Conditioner" to limit network speeds artificially.

@kgartland-rstudio
Copy link
Contributor

I was able to trigger the timeout but is there any way we can catch this exception and provide an error about how they increase the CONNECT_REQUEST_TIMEOUT if needed?

[DEBUG] 2023-03-22T11:43:56-0400 An exception occurred processing the HTTP request.
Traceback (most recent call last):
  File "/Users/kgartland/.pyenv/versions/3.8.2/envs/rsconnect-tag/lib/python3.8/site-packages/rsconnect/http_support.py", line 294, in _do_request
    self._conn.request(method, full_uri, body, headers)
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/http/client.py", line 1230, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/http/client.py", line 1276, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/http/client.py", line 1225, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/http/client.py", line 1004, in _send_output
    self.send(msg)
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/http/client.py", line 944, in send
    self.connect()
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/http/client.py", line 1399, in connect
    self.sock = self._context.wrap_socket(self.sock,
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/ssl.py", line 500, in wrap_socket
    return self.sslsocket_class._create(
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/ssl.py", line 1040, in _create
    self.do_handshake()
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/ssl.py", line 1309, in do_handshake
    self._sslobj.do_handshake()
socket.timeout: _ssl.c:1091: The handshake operation timed out
 	[ERROR]: Exception trying to connect to https://rsc.radixu.com - _ssl.c:1091: The handshake operation timed out
Error: Exception trying to connect to https://rsc.radixu.com - _ssl.c:1091: The handshake operation timed out

@tdstein
Copy link
Collaborator Author

tdstein commented Mar 22, 2023

I was able to trigger the timeout, but is there any way we can catch this exception and provide an error about how they increase the CONNECT_REQUEST_TIMEOUT if needed?

Sure! I will add an issue to the board and complete this in a separate pull request.

@tdstein tdstein merged commit c5e0d87 into master Mar 23, 2023
@tdstein tdstein deleted the 346-add-timeout-configuration branch March 23, 2023 14:53
@kgartland-rstudio
Copy link
Contributor

Validated the var works and timeout is rsconnect-python clients in Windows and MacOS.

  • default timeout is 300s
  • -v flag displays the HTTPConnection/HTTPSConnection configured
  • timeout is obeyed in both HTTP, HTTPS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add timeout configuration

6 participants