Add support for dmypy on Windows by emmatyping · Pull Request #5859 · python/mypy

emmatyping · 2018-10-31T05:25:18Z

After a while of banging my head against the Windows APIs today, I finally have most of dmypy running on Windows (wow this makes rechecks fast!).

~~Here is what works and what doesn't~~
Everything that is publicly documented should work now:

And here is a photo of it working :)

~~This depends on python/typeshed#2571. Until then selfcheck will fail (on Windows).~~

Closes #5019

Needed for #5859

emmatyping · 2018-10-31T14:17:51Z

Okay, I tested things on 3.4 and 3.7 and it works, so this should be ready for review.

gvanrossum

Sorry, I don't think I can finish this review. I hear Sully is looking at it. But I already created some comments so here they are.

emmatyping · 2018-11-07T08:34:36Z

Sorry, I don't think I can finish this review.

No problem, glad to get some feedback!

@msullivan happy to talk on Friday about this if you want.

msullivan

Thanks so much for this!

This all looks pretty plausible, and I think I'm basically happy with the daemonizing approach, but I'm pretty nervous about the named pipe stuff. I think duplicating the serve function will lead to mistakes and bugs done the road (and the windows version getting broken), especially since we don't actually have any automated tests for this IIRC.

I see two options here:

Abstract out all the opening/closing/sending logic for windows and unix into a library/object with a consistent interface, so we can have a single serve routine.
Just totally punt on using named pipes and switch to using TCP sockets (bound to localhost). There would be some difference between setting it up for domain sockets vs TCP sockets, but most of the socket code doesn't change at all. (Or we could honestly just always use TCP sockets, even on unix.)

My inclination is that we should do the latter, unless there is a major advantage of named pipes that I'm missing?

emmatyping · 2018-11-08T21:47:11Z

I think duplicating the serve function will lead to mistakes and bugs done the road (and the windows version getting broken)

Yeah, I can definitely understand that. The reason I chose NamedPipes was twofold: a) it is what Steve Dower recommended and b) its what mostly everyone else uses (at least from what I have seen). I am however not totally married to using them.

I see two options here
My inclination is that we should do the latter, unless there is a major advantage of named pipes that I'm missing?

Yeah, that is okay with me. I think moving entirely to TCP sockets makes the most sense, because then we can just share all IPC code across platforms, which makes the odds of things breaking very low.

msullivan · 2018-11-08T23:30:12Z

I think duplicating the serve function will lead to mistakes and bugs done the road (and the windows version getting broken)

Yeah, I can definitely understand that. The reason I chose NamedPipes was twofold: a) it is what Steve Dower recommended and b) its what mostly everyone else uses (at least from what I have seen). I am however not totally married to using them.

Yeah, in a very real sense they seem like they are the Right Thing for the job. I think the advantage of NamedPipes (and unix domain sockets) is that they have access control on them. This can matter on multi-user machines, but I think that multi-user windows dev machines aren't really a thing?

I see two options here
My inclination is that we should do the latter, unless there is a major advantage of named pipes that I'm missing?

Yeah, that is okay with me. I think moving entirely to TCP sockets makes the most sense, because then we can just share all IPC code across platforms, which makes the odds of things breaking very low.

I am somewhat inclined to keep using unix domain sockets when available, since multi-user Linux machines do still exist? It does mean some more conditional code, though I think a pretty small amount (just starting to listen and establishing the connection; all the communication should be the same).

msullivan · 2018-11-08T23:35:57Z

I am somewhat inclined to keep using unix domain sockets when available, since multi-user Linux machines do still exist? It does mean some more conditional code, though I think a pretty small amount (just starting to listen and establishing the connection; all the communication should be the same).

Though maybe this is a sign that we should stick with NamedPipe also? Though I think it does need to be abstracted out into a set of communication routines.

emmatyping · 2018-11-09T01:04:56Z

Though maybe this is a sign that we should stick with NamedPipe also? Though I think it does need to be abstracted out into a set of communication routines.

I think in the interest of not making assumptions about a user's environment, erring on the side of caution makes sense here. While multi-user Windows computers are much less common, I don't think we should preclude them. Changes to the actual IPC is likely going to be pretty rare, so I think a wrapper makes the most sense. It gives the added bonus that we can change the underlying implementation later on without needing to change any of the dmypy code.

gvanrossum · 2018-11-09T03:30:16Z

I didn't read the code, but I do agree that we should stick with unix domain sockets and named pipes, because of the access control they have. It shouldn't be hard to have a simple abstraction on top of these. (Though perhaps there might be a 3rd party package that does this?)

msullivan

I think I have now given all of the feedback I intend to.

Sorry for the slow drip, here. I spent a long time resisting the necessity of sucking it up and reading all the named pipe documentation.

emmatyping · 2018-11-20T07:54:21Z

Hm, no idea why the address would not be found in the tests, they are passing locally. I will investigate more tomorrow.

emmatyping · 2018-11-22T06:41:28Z

I'm chasing a flake in the end-to-end daemon tests on Windows, so I will be causing a bunch of commits trying to find what is possibly a race condition, sorry!

While thinking about what might cause flakes in #5859, I found a race condition between `dmypy stop` and other `dmypy` commands. `dmypy stop` will send a response and close the socket before the status file has been deleted. This means that there is a window in which the daemon has reported to a client that it has exited but while a status file still exists. This can result in a number of issues, including the daemon appearing to be stuck (instead of stopped) to subsequent commands and also the exiting server deleting the status file of a subsequently started server. The fix is to remove the status file in `cmd_stop` before replying to the request. This ensures that, as far as clients are concerned, the daemon is exited after a stop command completes. I tested the bug and the fix by inserting a `time.sleep(1)` immediately before the `sys.exit(0)` in `serve`: this caused several tests to fail, and the changes fixed them. I believe that the observed flakes in the windows version in #5859 were caused by this issue, but in a way that the unix version was not susceptible to.

emmatyping · 2018-11-27T02:11:16Z

Okay, I have run @msullivan's patch to fix the race condition 9 times on Appveyor and it passed every time. So I think we are finally rid of the flake caused by the race condition 🎉

msullivan

I have a few remaining nits, but this looks great and should be ready to land once addressed.

Thanks for all of the great work on this!

emmatyping added 8 commits October 25, 2018 21:07

Initial dmypy for Windows work

63aa4e2

Fixes related to dmypy server on Windows

abc64b7

Get dmypy start,stop,run,kill,check,restart working! 🎉

cf33bfa

Only use _winapi on Windows

54859da

Guard _winapi import dmypy_server

f760e16

Get dmypy status working on Windows

966117c

Add support for timeout

68ee937

Fix lint on Linux

f6a42c3

emmatyping commented Oct 31, 2018

View reviewed changes

Comment thread mypy/dmypy_server.py

emmatyping changed the title ~~[WIP] Add support for dmypy on Windows~~ Add support for dmypy on Windows Oct 31, 2018

emmatyping added 4 commits October 31, 2018 00:06

Change a while 1 to a while True

168e177

Minor refactoring

3e0d12e

Move conditional imports to end of imports

87e61c5

Minor fixes, corrections, simplifications

1883c31

emmatyping mentioned this pull request Oct 31, 2018

Sync Typeshed #5860

Merged

emmatyping added a commit that referenced this pull request Oct 31, 2018

Sync typeshed (#5860)

25e750b

Needed for #5859

Merge branch 'master' of github.com:python/mypy into windmypy

7932bb7

msullivan self-requested a review November 2, 2018 16:27

Remove outdated docs

8fb7203

gvanrossum reviewed Nov 6, 2018

View reviewed changes

Comment thread mypy/dmypy.py Outdated

Comment thread mypy/dmypy.py Outdated

Comment thread mypy/dmypy.py Outdated

Respond to Guido's comments

69d04dd

msullivan requested changes Nov 8, 2018

View reviewed changes

Comment thread mypy/dmypy.py

Comment thread mypy/dmypy_server.py Outdated

Lower timeout back down to 5s

968e07c

msullivan reviewed Nov 17, 2018

View reviewed changes

Comment thread mypy/ipc.py Outdated

Comment thread mypy/ipc.py Outdated

Comment thread mypy/ipc.py Outdated

msullivan reviewed Nov 17, 2018

View reviewed changes

Comment thread mypy/dmypy.py Outdated

emmatyping added 4 commits November 19, 2018 17:20

Merge branch master into ethanhs/windmypy

76e0539

Get tests passing on Windows

4956188

Merge branch 'windmypy' of github.com:ethanhs/mypy into windmypy

4e0dcc1

Try an empty string/quotes instead of single quotes

842712e

emmatyping added 6 commits November 20, 2018 14:05

Really fix daemon tests

5d9e149

Remove busy wait and give better debug info

959f89a

Temporary debug info

326758c

Reuse NamedPipe for multiple connections

d806282

Conditionally import Options in dmypy

989ca72

Remove debug info

da719d6

emmatyping force-pushed the windmypy branch 2 times, most recently from d75c230 to e97263b Compare November 22, 2018 06:31

emmatyping force-pushed the windmypy branch from c191245 to da719d6 Compare November 22, 2018 07:28

msullivan mentioned this pull request Nov 26, 2018

Fix a race condition in dmypy #5956

Merged

Merge upstream into windmypy

c2d8d90

msullivan approved these changes Nov 27, 2018

View reviewed changes

Comment thread mypy/dmypy.py Outdated

Comment thread mypy/dmypy_server.py

Comment thread mypy/ipc.py Outdated

Comment thread test-data/unit/daemon.test

Comment thread docs/source/mypy_daemon.rst

emmatyping added 5 commits November 27, 2018 14:27

Be more eager in removing status file

a732a1e

Remove redundant parens

dae10eb

Use random bytes for NamedPipe name

9de1b88

Document timeouts on Windows

f3b8bb6

Add line in note

0c43879

emmatyping merged commit f010360 into python:master Nov 27, 2018

emmatyping deleted the windmypy branch November 27, 2018 23:06

Conversation

emmatyping commented Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

emmatyping commented Oct 31, 2018

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emmatyping commented Nov 7, 2018

Uh oh!

msullivan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

emmatyping commented Nov 8, 2018

Uh oh!

msullivan commented Nov 8, 2018

Uh oh!

msullivan commented Nov 8, 2018

Uh oh!

emmatyping commented Nov 9, 2018

Uh oh!

gvanrossum commented Nov 9, 2018

Uh oh!

msullivan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emmatyping commented Nov 20, 2018

Uh oh!

emmatyping commented Nov 22, 2018

Uh oh!

emmatyping commented Nov 27, 2018

Uh oh!

msullivan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emmatyping commented Oct 31, 2018 •

edited

Loading