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

Update _reloader.py #2823

Merged
merged 1 commit into from
May 4, 2024
Merged

Update _reloader.py #2823

merged 1 commit into from
May 4, 2024

Conversation

momotarogrp
Copy link
Contributor

I've encountered a TypeError in the _find_common_roots function within the _reloader.py module, specifically triggered when os.path.join(*path) is called with an empty path tuple.

\werkzeug_reloader.py 161
root={}
path=()
os.path.join(*path)
Traceback (most recent call last):
File "", line 1, in
TypeError: join() missing 1 required positional argument: 'path'

  • fixes #

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@davidism
Copy link
Member

When is path empty? This needs a reproducing example.

@momotarogrp
Copy link
Contributor Author

https://cloud.google.com/anthos/run/archive/docs/tutorials/image-processing
This occurred while modifying this code to develop a cloudRun service.

I don't know the specific cause, but it seems to depend on the difference in containers in the execution environment.
It does not occur on instances deployed to cloudRun.

windows10 docker desctop Skaffold only occurs while debugging.
Unable to start program with debugger
There was no way to fix it, so I added check code to the library code and it started working properly.
Looking at the code, I think it would be more appropriate to have a check code.

@SnijderC
Copy link

I just disabled debug mode because of the same issue.

Paths are taken from os.path, which always contains a "" AFAIK. I can't not reproduce it.

docker run -it python:3.9
Python 3.9.18 (main, Feb 13 2024, 09:58:52)
[GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> assert '' in sys.path
>>>
docker run -it python:3.10
Python 3.10.13 (main, Feb 13 2024, 09:39:14) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> assert '' in sys.path
>>>
docker run -it python:3.11
Python 3.11.8 (main, Feb 13 2024, 09:03:56) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> assert '' in sys.path
>>>
docker run -it python:3.12
Python 3.12.2 (main, Feb 13 2024, 08:24:27) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> assert '' in sys.path
>>>

@ehiggs
Copy link

ehiggs commented Feb 16, 2024

I can also reproduce this from docker:

❯ docker run -it python:3.12-slim python
Python 3.12.2 (main, Feb 13 2024, 08:34:52) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path
['', '/usr/local/lib/python312.zip', '/usr/local/lib/python3.12', '/usr/local/lib/python3.12/lib-dynload', '/usr/local/lib/python3.12/site-packages']
>>> 

@SnijderC
Copy link

SnijderC commented Feb 16, 2024

BTW, the bug plays out like this:

  • Paths are taken from sys.path here, this function then makes a set of unique absolute paths and passes it to: _find_common_roots

  • _find_common_roots iterates over for chunks in sorted((PurePath(x).parts for x in paths), key=len, reverse=True):, where .parts leads to tuples of each path's parts. For PurePath("").parts this means: ().

  • Then a hierarchical structure of paths is generated recursively by assigning a reference to root to node and getting the current dict at each node in the hierarchy, or setting it to a new empty dict.

      def _find_common_roots(paths: t.Iterable[str]) -> t.Iterable[str]:
      root: dict[str, dict] = {}
    
      for chunks in sorted((PurePath(x).parts for x in paths), key=len, reverse=True):
          node = root
    
          for chunk in chunks:
              node = node.setdefault(chunk, {})
    
          node.clear()
    

    To get only the "common paths", node is cleared, only the parent directories remain in the hierarchy. However:

    • If "" is in sys.path it's at the start of it, since reverse=True is passed to the sorted method, it's handled last. Up to the point where the empty path is processed, everything looks normal, root is getting populated recursively.
    • Then when it's time for "" to be processed, whose "parts" are : (), then for chunk in (): will lead to node not referring to any chunk, which is thus still referring to root, which is subsequently cleared by node.clear(), effectively clearing root.
    • Next this block of code is run with an empty root:
     rv = set()
    
     def _walk(node: t.Mapping[str, dict], path: tuple[str, ...]) -> None:
         for prefix, child in node.items():
             _walk(child, path + (prefix,))
    
         if not node:
             rv.add(os.path.join(*path))
    
     _walk(root, ())
     return rv
    
  • walk not has nothing to walk, if not node: evaluates to True, path will still be the empty tuple passed to _walk, which is fed to os.path.join

@davidism davidism added this to the 3.0.3 milestone May 4, 2024
root={}
path=()
os.path.join(*path)
Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: join() missing 1 required positional argument: 'path'
@davidism davidism merged commit c58d391 into pallets:main May 4, 2024
11 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants