Opened 12 years ago

Closed 12 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 12 years ago by therve

Cc: therve added

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

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

Where did you see that?

comment:3 Changed 12 years ago by therve

Sorry I got confused.

comment:4 Changed 12 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 12 years ago by Jean-Paul Calderone

This is blocking the resolution of #745.

comment:7 Changed 12 years ago by therve

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

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

comment:8 Changed 12 years ago by therve

Keywords: review added
Owner: Glyph deleted

comment:9 Changed 12 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 12 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 12 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 11 years ago by <automation>

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