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.

Sign up for GitHub

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

Optimize checking of supported args #152

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Optimize checking of supported args #152

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ce49cf
optimize checking of supported args
matuskosut Sep 12, 2024
9c4eac7
Merge branch 'main' into rstudio-2024
matuskosut Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions jupyter_rsession_proxy/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import getpass
import os
import pathlib
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed pathlib since it is not used

import shutil
import subprocess
import tempfile
Expand Down Expand Up @@ -46,7 +45,7 @@ def rewrite_netloc(response, request):
def get_system_user():
try:
user = pwd.getpwuid(os.getuid())[0]
except:
except Exception:
user = os.getenv('NB_USER', getpass.getuser())
return(user)

Expand All @@ -73,9 +72,16 @@ def db_config(db_dir):
f.close()
return db_config_name

def _support_arg(arg):
def _support_args(args):
ret = subprocess.check_output([get_rstudio_executable('rserver'), '--help'])
return ret.decode().find(arg) != -1
help_output = ret.decode()
return {arg: (help_output.find(arg) != -1) for arg in args}

def _get_www_frame_origin(default="same"):
try:
return os.getenv('JUPYTER_RSESSION_PROXY_WWW_FRAME_ORIGIN', default)
except Exception:
return default
Comment on lines +81 to +84
Copy link
Member

Choose a reason for hiding this comment

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

I don't think a try/except block is relevant here, I don't know of an error to be raised that we want that can be raised in this situation using os.getenv.

Suggested change
try:
return os.getenv('JUPYTER_RSESSION_PROXY_WWW_FRAME_ORIGIN', default)
except Exception:
return default
return os.getenv('JUPYTER_RSESSION_PROXY_WWW_FRAME_ORIGIN', default)

Could you update the PR title or description to try capture this is now configurable via an environment variable? Whatever is put in the title will be visible via the changelog btw

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I've seen #148 now.

So this PR isn't really introducing this after all, its already there?


def _get_www_frame_origin(default="same"):
try:
Expand All @@ -102,13 +108,27 @@ def _get_cmd(port):
]
# Support at least v1.2.1335 and up

if _support_arg('www-root-path'):
supported_args = _support_args([
'www-root-path',
'server-data-dir',
'database-config-file',
'www-thread-pool-size',
])
if supported_args['www-root-path']:
cmd.append('--www-root-path={base_url}rstudio/')
if _support_arg('server-data-dir'):
if supported_args['server-data-dir']:
cmd.append(f'--server-data-dir={server_data_dir}')
if _support_arg('database-config-file'):
if supported_args['database-config-file']:
cmd.append(f'--database-config-file={database_config_file}')

if supported_args['www-thread-pool-size']:
try:
thread_pool_size = int(os.getenv('RSERVER_THREAD_POOL_SIZE', ""))
if thread_pool_size > 0:
cmd.append('--www-thread-pool-size=' + str(thread_pool_size))
except Exception:
pass

Comment on lines +125 to +131
Copy link
Member

@consideRatio consideRatio Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If this is errorously configured, such as having RSERVER_THREAD_POOL_SIZE set to "asdf", I think it can make sense to error loudly instead. Of course, if its an empty string though, we shouldn't error, but we can check that without a try/except block.

I think maybe something like...

int(os.getenv('RSERVER_THREAD_POOL_SIZE') or "0") can do the trick, assuming we only want to configure --ww-thread-pool-size if its set to something else than 0.

return cmd

def _get_timeout(default=15):
Expand Down