tomv564 / pyls-mypy

Mypy plugin for the Python Language Server
MIT License
110 stars 62 forks source link

mypy errors are not captured and end up directly in the process stdout #19

Open randomstuff opened 5 years ago

randomstuff commented 5 years ago

While debugging an error appearing in Atom IDE-Python plugin, we found that some errors reported by mypy sometimes end up appearing directly in the process stdout (outside on the JSON body) which breaks the protocol:

Content-Length: 43
Content-Type: application/vscode-jsonrpc; charset=utf8

{"jsonrpc": "2.0", "id": 5, "result": null}
:1:1: error: Cannot find module named 'cryptography.hazmat.primitives'
:1:1: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
:2:1: error: Cannot find module named 'cryptography.hazmat.primitives.kdf.hkdf'
:3:1: error: Cannot find module named 'cryptography.hazmat.backends.openssl'
:4:1: error: Cannot find module named 'cryptography.hazmat.primitives.ciphers'
:11:1: error: Cannot find module named 'filetype'

You can easily check the output of pyls with sysdig:

sysdig -s6000 'proc.cmdline="python -m pyls"' -c stdout

AFAIU, this happens because mypy API currently works by temporarily overriding sys.stdout and sys.stderr which is not thread-safe:

def _run(f: Callable[[], None]) -> Tuple[str, str, int]:
    old_stdout = sys.stdout
    new_stdout = StringIO()
    sys.stdout = new_stdout

    old_stderr = sys.stderr
    new_stderr = StringIO()
    sys.stderr = new_stderr

    try:
        f()
        exit_status = 0
    except SystemExit as system_exit:
        exit_status = system_exit.code
    finally:
        sys.stdout = old_stdout
        sys.stderr = old_stderr

    return new_stdout.getvalue(), new_stderr.getvalue(), exit_status

def run(args: List[str]) -> Tuple[str, str, int]:
    # Lazy import to avoid needing to import all of mypy to call run_dmypy
    from mypy.main import main
    return _run(lambda: main(None, args=args))

The linter tasks are handled in separate threads because of the debouncing feature of python-language-server:

def debounce(interval_s, keyed_by=None):
    """Debounce calls to this function until interval_s seconds have passed."""
    def wrapper(func):
        timers = {}
        lock = threading.Lock()

        @functools.wraps(func)
        def debounced(*args, **kwargs):
            call_args = inspect.getcallargs(func, *args, **kwargs)
            key = call_args[keyed_by] if keyed_by else None

            def run():
                with lock:
                    del timers[key]
                return func(*args, **kwargs)

            with lock:
                old_timer = timers.get(key)
                if old_timer:
                    old_timer.cancel()

                timer = threading.Timer(interval_s, run)
                timers[key] = timer
                timer.start()
        return debounced
    return wrapper

Because stdout overriding is not thread safe, sys.stout may not be correctly restored and some errors are actually reported in the real/original/system stdout instead of the StringIO one.

randomstuff commented 5 years ago

One workaround might be to use a mutex in order to prevent concurrent mypy linting. Ultimately, sys.stdout and sys.stderr overriding could create some other issues and this should be fixed in mypy API instead.

elkhadiy commented 5 years ago

This will most likely be resolved in python/mypy#6125. As I said in python/mypy/pull/6129 if people are having trouble with this, in the meantime, a quick duct-tape solution could be to replace the call of mypy.api.run with something like this:

res = subprocess.run(
    ["python", "-m", "mypy", *args],
    stdout=subprocess.PIPE, stderr=subprocess.PIPE
    )
report = res.stdout
errors = res.stderr
tomv564 commented 5 years ago

@randomstuff amazing tenacity narrowing own the cause of this issue!

Let me know if we need an alternative to the pull request solution.

randomstuff commented 5 years ago

@elkhadiy started working on a PR on mypy. (@elkhadiy: Tell me if you want some help on the PR.)

In the meanwhile, I used the workaround of executing the body of _run() while holding a mutex:

_lock = Lock()

def _run(f: Callable[[], None]) -> Tuple[str, str, int]:
    with _lock:
        old_stdout = sys.stdout
        new_stdout = StringIO()
        sys.stdout = new_stdout

        old_stderr = sys.stderr
        new_stderr = StringIO()
        sys.stderr = new_stderr

        try:
            f()
            exit_status = 0
        except SystemExit as system_exit:
            exit_status = system_exit.code
        finally:
            sys.stdout = old_stdout
            sys.stderr = old_stderr

        return new_stdout.getvalue(), new_stderr.getvalue(), exit_status

It's working alright but I don't think it's worth merging this hack as the correct fix (in mypy) should be comparatively simple.

remorses commented 5 years ago

upgrading mypy to the latest version now solves the issue

edit: no it does not

randomstuff commented 5 years ago

@remorses, mypy's bug is not marked as fixed and at first glance I don't see any change in the latest release in this regard.

hrouault commented 2 years ago

This issue appeared when I activated dmypy in the configuration file:

Here is my pylsp-mypy.cfg:

{
    "enabled": True,
    "live_mode": False,
    "dmypy": True,
    "strict": False
}

When I set dmypy to False, the error disappears