avoid using win32eventreactor, or suppress spurious warning from it if necessary

[zooko]

FreeStorm's WinXP buildslave emits this warning (log file):

c:\python26\lib\site-packages\twisted\internet\win32eventreactor.py:64: UserWarning: Reliable disconnection notification requires pywin32 215 or later

Here's the ​code in Twisted that emits that warning.

Here's the ​patch in Twisted that added this warning (in 2011-11-02).

Here's the ​ticket in Twisted about the issue.

This might mean that we need to undo our removal of the dependency on pywin32 (#1274), because this newer version of Twisted requires pywin32 in order to do something that we want. (Actually, older versions of Twisted just don't do that thing -- they don't notice sometimes on Windows when a TCP connection is closed -- so this isn't a regression, except for the presence of the warning, which we could suppress.)

I really hope we don't have to reintroduce a dependency on pywin32! We worked long and hard to get rid of that dependency. Adding it back in would increase the number of software packages that a Tahoe-LAFS user has to manually install from two (CPython, Tahoe-LAFS) to three (CPython, pywin32, Tahoe-LAFS). It would also cause all Tahoe-LAFS users, regardless of whether they use Windows or not, to see some text saying "If you are using Windows, install pywin32 as follows..." in docs/quickstart.rst, or else to see some text saying "If you are using Windows, go to this other page to read some platform-specific instructions". I strongly wish to avoid all of the above.

Another alternative is to monkeypatch twisted to use a C extension module, such as is attached to the twisted ticket, or ctypes instead of pywin32.

Another alternative is not to rely on disconnection notification! In my opinion, code which relies on disconnection notification is almost never necessary and is often incorrect, since disconnection notification can never be really reliable (even if both the foolscap bug and the Twisted/pywin32 bug were fixed).

We've already removed almost all such reliance on disconnection notification above the foolscap layer from Tahoe-LAFS. Our motivation in doing that was the discovery of a bug, that we have never yet fixed, in foolscap's notifyOnDisconnect, but also it was in part motivated by my belief that disconnection-notification is an anti-pattern -- a feature that it is often better to do without

There is one remaining place in Tahoe-LAFS where we rely on foolscap's notifyOnDisconnect -- in the user interface to show the user which servers are currently connected. see #816 for the task of removing that place. There is also the fact that Brian may want to rely on disconnection-notification as an optimization in upload -- see #816 comment 1.

Finally, there is the possibility that this issue could cause problems for our code even though our code doesn't rely on foolscap-level notifyOnDisconnect. For example, what about the HTTP connections between the web browser and the Tahoe-LAFS gateway? I would guess that failure to notice a disconnect will not cause a problem for any of our code (because, like I say, noticing disconnects and responding to them usefully is a dicey proposition at the best of times) but we should probably think it through if we're going to suppress the warning and commit to relying on a version of Twisted which doesn't notice (some) disconnects on Windows.

[itamarst]

I don't understand this ticket at all.

1. If you're using win32eventreactor, you need pywin32, that's how it's implemented. So if you require win32eventreactor you're already requiring pywin32, the change in Twisted doesn't change anything for you. What changed is that it suggests a *modern* version of pywin32 for use with win32eventreactor.
2. On the other hand, you don't actually need to use win32eventreactor on Windows, there's the default select() reactor, or the IOCP reactor which scales much better.

This ticket seems invalid to me.

[davidsarah]

We shouldn't be using win32eventreactor. If we end up using it, that's a bug, since it depends on pywin32 which we don't want to do [*]. The intention is to use the default select reactor on Windows. So why is that warning from win32eventreactor being emitted on FreeStorm's XP buildslave, when there is no reference to it in Tahoe code? Is one of our dependencies sneakily referencing it, or is Twisted emitting the warning even when the reactor is not used (which would be a Twisted bug)?

[*] I just noticed that src/allmydata/test/common_util.py​ still has a dependency on pywin32 (by importing win32file and win32con), although it falls back to something else if it is not present. We should probably fix that.

[davidsarah]

Running Python with verbose import tracing (-v or the PYTHONVERBOSE env var) might help to track down what is importing win32eventreactor.

[davidsarah]

I can't straightforwardly reproduce this on my WinXP VM (but I only tried from twisted.internet import reactor using Twisted 12.0 without pywin32 installed, which might not be sufficient to reproduce it anyway).

[zooko]

Itamar: thanks for disabusing me of my confusions. I'm slightly embarrassed, but relieved. davidsarah: good thinking in comment:2, comment:3, comment:4.

[itamarst]

Oh, actually: the default select() reactor will attempt to load some of the win32eventreactor code in order to better support certain functionality on Windows, e.g. serial ports. (It tries to run a parallel win32event loop in a thread). This is probably why you're seeing the warning even if you're using the select() reactor. If, as davidsarah is seeing, pywin32 is not installed this additional functionality will be skipped.

So, you can just ignore that warning: it will only show up for people who already have pywin32 installed, in which case telling them they maybe want to upgrade isn't that big deal.

[davidsarah]

Replying to itamarst:

Oh, actually: the default select() reactor will attempt to load some of the win32eventreactor code in order to better support certain functionality on Windows, e.g. serial ports. (It tries to run a parallel win32event loop in a thread).

In that case I think we should suppress the warning, since, if I understand correctly, we don't use any functionality that would be affected by it.

So, you can just ignore that warning: it will only show up for people who already have pywin32 installed, in which case telling them they maybe want to upgrade isn't that big deal.

That would be anyone upgrading from a version of Tahoe prior to v1.8.2, in which we (mostly) removed the pywin32 dependencency. So it is quite a big deal, since it requires us to document this possibility for everyone.

[itamarst]

It seems like the warnings module has the necessary infrastructure to do so, so yeah, that's probably what you should do if you're worried about user reactions.

[davidsarah]

Suppress a warning from win32eventreactor on Windows (patch v2). fixes #1681

[zooko]

Reviewed! Try as I might, I could find no flaw in it.

[david-sarah@…]

In 916d26e7103208fa:

Suppress a warning from win32eventreactor on Windows (patch v2). fixes #1681