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.

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

It is possible for python_tzpath_context to fail in test_zoneinfo #102537

Closed
pganssle opened this issue Mar 8, 2023 · 2 comments
Closed

It is possible for python_tzpath_context to fail in test_zoneinfo #102537

pganssle opened this issue Mar 8, 2023 · 2 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@pganssle
Copy link
Member

pganssle commented Mar 8, 2023

Bug report

This code started failing the pylint check on backports.zoneinfo recently:

@staticmethod
@contextlib.contextmanager
def python_tzpath_context(value):
    path_var = "PYTHONTZPATH"
    try:
        with OS_ENV_LOCK:
           old_env = os.environ.get(path_var, None)
           os.environ[path_var] = value
           yield
    finally:
        if old_env is None:
            del os.environ[path_var]
        else:
            os.environ[path_var] = old_env  # pragma: nocover

It's kind of a non-issue, but it is true that if an error is raised while acquiring OS_ENV_LOCK or when calling os.environ.get, old_env won't be set, which will raise a NameError. This is a kind of rare situation anyway, and it would probably only come up during a non-recoverable error, but we may as well fix it — if only so that people who use CPython as an example of "good code" will have an example of the right way to handle this sort of situation.

Probably this is trivial enough that we can skip an issue and news, but I'll just create one anyway 😛

Linked PRs

@pganssle pganssle added the type-bug An unexpected behavior, bug, or error label Mar 8, 2023
miss-islington pushed a commit that referenced this issue Mar 10, 2023
…-102538)

It is possible but unlikely for the `python_tzpath_context` function to fail between the start of the `try` block and the point where `os.environ.get` succeeds, in which case `old_env` will be undefined. In this case, we want to take no action.

Practically speaking this will really only happen in an error condition anyway, so it doesn't really matter, but we should probably do it right anyway.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Mar 10, 2023
…st (pythonGH-102538)

It is possible but unlikely for the `python_tzpath_context` function to fail between the start of the `try` block and the point where `os.environ.get` succeeds, in which case `old_env` will be undefined. In this case, we want to take no action.

Practically speaking this will really only happen in an error condition anyway, so it doesn't really matter, but we should probably do it right anyway.
(cherry picked from commit 64bde50)

Co-authored-by: Paul Ganssle <1377457+pganssle@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Mar 10, 2023
…st (pythonGH-102538)

It is possible but unlikely for the `python_tzpath_context` function to fail between the start of the `try` block and the point where `os.environ.get` succeeds, in which case `old_env` will be undefined. In this case, we want to take no action.

Practically speaking this will really only happen in an error condition anyway, so it doesn't really matter, but we should probably do it right anyway.
(cherry picked from commit 64bde50)

Co-authored-by: Paul Ganssle <1377457+pganssle@users.noreply.github.com>
miss-islington added a commit that referenced this issue Mar 10, 2023
…-102538)

It is possible but unlikely for the `python_tzpath_context` function to fail between the start of the `try` block and the point where `os.environ.get` succeeds, in which case `old_env` will be undefined. In this case, we want to take no action.

Practically speaking this will really only happen in an error condition anyway, so it doesn't really matter, but we should probably do it right anyway.
(cherry picked from commit 64bde50)

Co-authored-by: Paul Ganssle <1377457+pganssle@users.noreply.github.com>
miss-islington added a commit that referenced this issue Mar 13, 2023
…-102538)

It is possible but unlikely for the `python_tzpath_context` function to fail between the start of the `try` block and the point where `os.environ.get` succeeds, in which case `old_env` will be undefined. In this case, we want to take no action.

Practically speaking this will really only happen in an error condition anyway, so it doesn't really matter, but we should probably do it right anyway.
(cherry picked from commit 64bde50)

Co-authored-by: Paul Ganssle <1377457+pganssle@users.noreply.github.com>
@arhadthedev
Copy link
Member

Should this issue be closed as resolved?

@hugovk
Copy link
Member

hugovk commented Nov 10, 2023

Yes, let's close it :)

@hugovk hugovk closed this as completed Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants