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

Add support for dmypy on Windows #5859

Merged
merged 56 commits into from Nov 27, 2018

Conversation

Projects
None yet
3 participants
@ethanhs
Copy link
Collaborator

ethanhs commented Oct 31, 2018

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:

  • dmypy start
  • dmypy stop
  • dmypy restart
  • dmypy run
  • dmypy kill
  • dmypy check
  • dmypy status

And here is a photo of it working :)
image

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

Closes #5019

ethanhs added some commits Oct 26, 2018

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

ethanhs added some commits Oct 31, 2018

@ethanhs ethanhs referenced this pull request Oct 31, 2018

Merged

Sync Typeshed #5860

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

Sync typeshed (#5860)
Needed for #5859
@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Oct 31, 2018

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

@msullivan msullivan self-requested a review Nov 2, 2018

@gvanrossum
Copy link
Member

gvanrossum left a comment

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.

Show resolved Hide resolved mypy/dmypy.py Outdated
Show resolved Hide resolved mypy/dmypy.py Outdated
Show resolved Hide resolved mypy/dmypy.py Outdated
@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Nov 7, 2018

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
Copy link
Collaborator

msullivan left a comment

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?

Show resolved Hide resolved mypy/dmypy.py Outdated
Show resolved Hide resolved mypy/dmypy_server.py Outdated
@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Collaborator

msullivan commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Collaborator

msullivan commented Nov 8, 2018

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.

@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Nov 9, 2018

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

This comment has been minimized.

Copy link
Member

gvanrossum commented Nov 9, 2018

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?)

ethanhs added some commits Nov 20, 2018

@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Nov 20, 2018

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

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented on test-data/unit/daemon.test in 4956188 Nov 20, 2018

This won't work on UNIX, alas -- '#' is a shell comment character so it will not append to plug.py. IIRC I couldn't write 'pass' because plug.py doesn't end in a newline. Though maybe plain 'echo' will print at least a newline? That should be sufficient.

This comment has been minimized.

Copy link
Collaborator

ethanhs replied Nov 20, 2018

Ah, yeah I realized this last night. I think I will just use Python for this and template it in.

This comment has been minimized.

Copy link
Member

gvanrossum replied Nov 20, 2018

If plain echo >>plug.py works, that would be fine -- it works on UNIX.

This comment has been minimized.

Copy link
Collaborator

ethanhs replied Nov 20, 2018

FWIW, echo on Windows returns ECHO is on., so I cannot use that sadly.

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented on 5d9e149 Nov 20, 2018

OK!

ethanhs added some commits Nov 20, 2018

@ethanhs ethanhs force-pushed the ethanhs:windmypy branch 2 times, most recently from d75c230 to e97263b Nov 22, 2018

@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Nov 22, 2018

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!

@ethanhs ethanhs force-pushed the ethanhs:windmypy branch from c191245 to da719d6 Nov 22, 2018

msullivan added a commit that referenced this pull request Nov 26, 2018

Fix a race condition in dmypy
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.

msullivan added a commit that referenced this pull request Nov 27, 2018

Fix a race condition in dmypy (#5956)
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.
@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Nov 27, 2018

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
Copy link
Collaborator

msullivan left a comment

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!

Show resolved Hide resolved mypy/dmypy.py Outdated
Show resolved Hide resolved mypy/dmypy_server.py
Show resolved Hide resolved mypy/ipc.py Outdated
Show resolved Hide resolved test-data/unit/daemon.test
Show resolved Hide resolved docs/source/mypy_daemon.rst

ethanhs added some commits Nov 27, 2018

@ethanhs ethanhs merged commit f010360 into python:master Nov 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ethanhs ethanhs deleted the ethanhs:windmypy branch Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment