classification
Title: Problem with contextlib.nullcontext
Type: resource usage Stage: patch review
Components: Versions: Python 3.8, Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: eric.smith, miss-islington, ncoghlan, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2018-07-07 17:33 by serhiy.storchaka, last changed 2018-07-11 15:03 by ncoghlan.

Pull Requests
URL Status Linked Edit
PR 8158 merged banool, 2018-07-09 13:23
PR 8199 merged miss-islington, 2018-07-09 13:50
Messages (7)
msg321226 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-07 17:33
There is a flaw in the example in the documentation for contextlib.nullcontext.

      def process_file(file_or_path):
          if isinstance(file_or_path, str):
              # If string, open file
              cm = open(file_or_path)
          else:
              # Caller is responsible for closing file
              cm = nullcontext(file_or_path)

          with cm as file:
              # Perform processing on the file

The execution can be interrupted by Ctrl-C between opening a file and entering a 'with' block. There is the same problem with the simple "with open(path) as file:", but it can be easily solved (see issue34066). It is harder to do for this example, because there are more opcodes between calling open() and entering 'with'. I afraid that this problem can't be solved in general case. Maybe contextlib.nullcontext has a flaw similar to the flaw in contextlib.nested().
msg321254 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-08 04:14
That's certainly similar to the problems with contextlib.nested, but I don't think it's as inherently flawed as nested was. What I'd suggest we change the existing example to is this:

      from functools import partial
      from os import fspath

      def process_file(file_or_path):
          try:
              as_fspath = os.fspath(file_or_path)
          except TypeError:
              # If it's a file, caller is responsible for closing it
              make_cm = partial(nullcontext, file_or_path)
          else:
              # If it's a path, open file when the context is entered
              make_cm = partial(open, as_fspath)

          with make_cm() as file:
              # Perform processing on the file

Optionally, we could also present a cleaner example where a pre-created context manager is passed in and we're just coping with the fact it may be None:

    def update_resource(resource, updates, resource_lock=None):
        if resource_lock is None:
            resource_lock = nullcontext()
        with resource_lock:
            resource.apply_updates(updates)

(I'll also note that ExitStack is *far* from being immune to the Ctrl-C problem, as it's implemented in Python itself, which allows its __exit__ method to be interrupted, as well as for interrupt to occur between a resource being created or acquired, and it being registered with the stack)
msg321319 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-09 13:49
New changeset c287545d62edf1a1ee65727d3c57befa8c99c13a by Nick Coghlan (Daniel Porteous) in branch 'master':
bpo-34067: Include a more easily understood example for nullcontext (GH-8158)
https://github.com/python/cpython/commit/c287545d62edf1a1ee65727d3c57befa8c99c13a
msg321322 - (view) Author: miss-islington (miss-islington) Date: 2018-07-09 14:11
New changeset 1b6e21730417c56748d500a96cafebbaa1bd0120 by Miss Islington (bot) in branch '3.7':
bpo-34067: Include a more easily understood example for nullcontext (GH-8158)
https://github.com/python/cpython/commit/1b6e21730417c56748d500a96cafebbaa1bd0120
msg321461 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-11 14:37
Can we close this issue?
msg321463 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-11 14:39
No, the merged PR is not directly related to the original issue.
msg321472 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-07-11 15:03
Right, the merged PR adds a simpler example that handles "ignore exceptions or not" by selecting between suppress() and nullcontext().

The documentation still needs updating to improve or remove the file-based example, and point out that for resources that get acquired on construction, you either need to delay construction to the start of the with statement, or else use ExitStack instead of nullcontext.
History
Date User Action Args
2018-07-11 15:03:41ncoghlansetmessages: + msg321472
2018-07-11 14:39:42serhiy.storchakasetmessages: + msg321463
2018-07-11 14:37:48vstinnersetnosy: + vstinner
messages: + msg321461
2018-07-09 14:11:47miss-islingtonsetnosy: + miss-islington
messages: + msg321322
2018-07-09 13:50:41miss-islingtonsetpull_requests: + pull_request7752
2018-07-09 13:49:33ncoghlansetmessages: + msg321319
2018-07-09 13:23:56banoolsetkeywords: + patch
stage: patch review
pull_requests: + pull_request7751
2018-07-08 04:14:22ncoghlansetmessages: + msg321254
2018-07-08 00:45:16eric.smithsetnosy: + eric.smith
2018-07-07 17:33:25serhiy.storchakacreate