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

Jump to bottom

Add some minimal support for increasing the soft limit on open file handles, default to at least 4096 #4893

Merged
merged 4 commits into from
Oct 21, 2019
Merged

Add some minimal support for increasing the soft limit on open file handles, default to at least 4096 #4893

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7b953b5
Add some minimal support for increasing the soft limit on open file h…
jasongrout Sep 16, 2019
da2fd63
Only import resource in a try/except block
jasongrout Sep 16, 2019
2c65cdc
Log open file limit message before attempting to do so
jasongrout Sep 30, 2019
a119872
Merge branch 'master' into open_files
jasongrout Oct 16, 2019
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
33 changes: 32 additions & 1 deletion notebook/notebookapp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,13 @@
import warnings
import webbrowser

from base64 import encodebytes
try:
import resource
except ImportError:
# Windows
resource = None

from base64 import encodebytes

from jinja2 import Environment, FileSystemLoader

Expand Down Expand Up @@ -794,6 +799,14 @@ def _token_default(self):
"""
)

min_open_files_limit = Integer(4096, config=True,
Copy link
Member

@mpacer mpacer Oct 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Feel free to ignore this suggestion, but I think the traitlet name would be clearer if it were soft_open_files_limit. The reason I suggest this is that this is a soft bound on the maximum number of files that can be created, rather than a minimum number of files that must be opened.

Copy link
Member

@mpacer mpacer Oct 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

To be fair, you could call it simply open_files_limit as well, since in practice we also increase the hard limit if the soft limit being set exceeds the existing hard limit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I called it min_open_files_limit because I only set it if the current open files limit is lower than this number (i.e., I will never decrease an open files limit to this number, only raise lower open file limits to at least this.)

https://github.com/jupyter/notebook/pull/4893/files#diff-bbd307804bb6b8836da9f06b8d8a06e9R1405

That said, I also think the name is confusing, but descriptive, and would love an easier to understand name.

help="""
Gets or sets a lower bound on the open file handles process resource
limit. This may need to be increased if you run into an
OSError: [Errno 24] Too many open files.
This is not applicable when running on Windows.
""")

@observe('token')
def _token_changed(self, change):
self._token_generated = False
Expand Down Expand Up @@ -1371,6 +1384,23 @@ def init_logging(self):
logger.parent = self.log
logger.setLevel(self.log.level)

def init_resources(self):
"""initialize system resources"""
if resource is None:
self.log.debug('Ignoring min_open_files_limit because the limit cannot be adjusted (for example, on Windows)')
return

old_soft, old_hard = resource.getrlimit(resource.RLIMIT_NOFILE)
soft = self.min_open_files_limit
hard = old_hard
if old_soft < soft:
if hard < soft:
hard = soft
self.log.debug(
'Raising open file limit: soft {}->{}; hard {}->{}'.format(old_soft, soft, old_hard, hard)
)
resource.setrlimit(resource.RLIMIT_NOFILE, (soft, hard))
Copy link

Choose a reason for hiding this comment

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

According to the documentation (https://docs.python.org/3/library/resource.html):

Raises ValueError if an invalid resource is specified, if the new soft limit exceeds the hard limit, or (if a process tries to raise its hard limit. ... A process with the effective UID of super-user can request any valid limit value

So it seems this will fail for non-root


Copy link
Member

Choose a reason for hiding this comment

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

This strikes me as more of an informational message than debug. I would also suggest taking an optimistic approach and logging 'Raising open file...' prior to the actual call. This way, the log will contain the values that were presumably bad and could prove helpful in troubleshooting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Very good point!

Copy link
Member Author

Choose a reason for hiding this comment

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

I went ahead and left it a debug message, but moved it before the action. My thinking is that for most people, this should never be a concern, so don't clutter up their terminal. However, if you are trying to track down an issue, likely you are running with --debug.

def init_webapp(self):
"""initialize tornado webapp and httpserver"""
self.tornado_settings['allow_origin'] = self.allow_origin
Expand Down Expand Up @@ -1665,6 +1695,7 @@ def initialize(self, argv=None):
self.init_logging()
if self._dispatching:
return
self.init_resources()
self.init_configurables()
self.init_server_extension_config()
self.init_components()
Expand Down