Opened 8 years ago

Closed 7 years ago

#3925 defect closed fixed (fixed)

test_addresses fails with Glib2 and Gtk2 reactors

Reported by: Jean-Paul Calderone Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: therve Branch: branches/gtk2-sources-3925
branch-diff, diff-cov, branch-cov, buildbot
Author: therve


twisted.internet.test.test_tcp.TCPClientTestsBuilder_Glib2Reactor.test_addresses and twisted.internet.test.test_tcp.TCPClientTestsBuilder_Gtk2Reactor.test_addresses have been failing on the Windows builders since that host was updated.

The old versions of gtk and pygtk, with which those tests were passing, were 2.12.9 and 2.12.1 respectively (this can be seen on an old build's version output, eg <>).

The new versions, under which the tests fail, are 2.14.7 and 2.12.1.

This suggests there may have been a Gtk regression. I haven't investigated, though.

Change History (12)

comment:1 Changed 7 years ago by therve

Cc: therve added

It looks like it's failing with the select reactor as well now.

comment:2 Changed 7 years ago by Jean-Paul Calderone

Where did you see that?

comment:3 Changed 7 years ago by therve

Sorry I got confused.

comment:4 Changed 7 years ago by therve

The following changeset looks highly suspect, though I'm not really able to spot the error not have the build system to try to fix it:

2007-07-06  Tor Lillqvist  <>
        * glib/giowin32.c (g_io_win32_check): When WSAEnumNetworkEvents()
        signals FD_CONNECT that means that the connection attempt
        finished, either successfully or failed. Test explicitly whether
        the connnection succeeded and set either G_IO_OUT if it did,
        G_IO_ERR|G_IO_HUP if it failed.
        Make sure we never set both G_IO_OUT and G_IO_HUP simultaneously
        because in Unix poll(2) POLLOUT and POLLHUP are mutually
        Ignore whether the caller wants to watch G_IO_HUP or not. Always
        select for FD_CLOSE because Unix poll(2) also ignores whether
        POLLHUP in set the requested events bitmask or not.
    svn path=/trunk/; revision=5600

diff --git a/glib/giowin32.c b/glib/giowin32.c
index 8632209..83cb86c 100644
--- a/glib/giowin32.c
+++ b/glib/giowin32.c
@@ -822,8 +822,7 @@ g_io_win32_prepare (GSource *source,
        event_mask |= (FD_READ | FD_ACCEPT);
       if (watch->condition & G_IO_OUT)
        event_mask |= (FD_WRITE | FD_CONNECT);
-      if (watch->condition & G_IO_HUP)
-       event_mask |= FD_CLOSE;
+      event_mask |= FD_CLOSE;
       if (channel->event_mask != event_mask /* || channel->event != watch->pollfd.fd*/)
@@ -936,13 +935,32 @@ g_io_win32_check (GSource *source)
       watch->pollfd.revents = 0;
       if (channel->last_events & (FD_READ | FD_ACCEPT))
        watch->pollfd.revents |= G_IO_IN;
-      if (channel->last_events & (FD_WRITE | FD_CONNECT))
+      if (channel->last_events & FD_WRITE)
        watch->pollfd.revents |= G_IO_OUT;
-      if (watch->pollfd.revents == 0 && (channel->last_events & (FD_CLOSE)))
-       watch->pollfd.revents |= G_IO_HUP;
+      else
+       {
+         /* We have called WSAEnumNetworkEvents() above but it didn't
+          * set FD_WRITE.
+          */
+         if (events.lNetworkEvents & FD_CONNECT)
+           {
+             if (events.iErrorCode[FD_CONNECT_BIT] == 0)
+               watch->pollfd.revents |= G_IO_OUT;
+             else
+               watch->pollfd.revents |= (G_IO_HUP | G_IO_ERR);
+           }
+         if (watch->pollfd.revents == 0 && (channel->last_events & (FD_CLOSE)))
+           watch->pollfd.revents |= G_IO_HUP;
+       }
-      if (!channel->write_would_have_blocked && (channel->event_mask & FD_WRITE))
-       watch->pollfd.revents |= G_IO_OUT; /* This sucks but... */
+      /* Regardless of WSAEnumNetworkEvents() result, if watching for
+       * writability, unless last write would have blocked set
+       * G_IO_OUT. But never set both G_IO_OUT and G_IO_HUP.
+       */
+      if (!(watch->pollfd.revents & G_IO_HUP) &&
+         !channel->write_would_have_blocked &&
+         (channel->event_mask & FD_WRITE))
+       watch->pollfd.revents |= G_IO_OUT;
       return ((watch->pollfd.revents | buffer_condition) & watch->condition);

comment:6 Changed 7 years ago by Jean-Paul Calderone

This is blocking the resolution of #745.

comment:7 Changed 7 years ago by therve

Author: therve
Branch: branches/gtk2-sources-3925

(In [28322]) Branching to 'gtk2-sources-3925'

comment:8 Changed 7 years ago by therve

Keywords: review added
Owner: Glyph deleted

comment:9 Changed 7 years ago by Jamu Kakar

Keywords: review removed
Owner: set to therve


Please add docstrings to the following methods:

_add addReader addWriter getReaders getWriters removeAll _remove removeReader removeWriter

I see that IReactorFDSet has nice documentation for the public methods I listed above. Given this, maybe docstrings aren't necessary, but if nothing else it'd be nice to have a link to the interface so that you can get to it easily in the API documentation.

Other than that, the changes look fine to me.

comment:10 Changed 7 years ago by Jean-Paul Calderone

  1. There should be something added to the Gtk2Reactor class docstring for the new _sources attribute as well.
  2. In _remove there's a line, if not source in primary: which would be slightly nicer as if source not in primary: I think, though the behavior is the same either way.
  3. twisted.internet.gtk2reactor.Gtk2Reactor.getWriters isn't valid epytext

comment:11 Changed 7 years ago by therve

Resolution: fixed
Status: newclosed

(In [28356]) Merge gtk2-sources-3925

Author: therve Reviewers: jkakar, exarkun Fixes #3925

Change the Gtk2Reactor to keep only one watch per FD source instead of one for reading and one for writing. It fixes one misbehavior under Windows.

comment:12 Changed 6 years ago by <automation>

Owner: therve deleted
Note: See TracTickets for help on using tickets.