Opened 5 years ago

Closed 5 years ago

#3925 defect closed fixed (fixed)

test_addresses fails with Glib2 and Gtk2 reactors

Reported by: exarkun Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: therve Branch: branches/gtk2-sources-3925
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

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 <http://buildbot.twistedmatrix.com/builders/winxp32-py2.5-select/builds/450/steps/shell/logs/versions>).

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

  • Cc therve added

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

comment:2 Changed 5 years ago by exarkun

Where did you see that?

comment:3 Changed 5 years ago by therve

Sorry I got confused.

comment:4 Changed 5 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  <tml@novell.com>
    
        * 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
        exclusive.
    
        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 5 years ago by exarkun

This is blocking the resolution of #745.

comment:7 Changed 5 years ago by therve

  • Author set to therve
  • Branch set to branches/gtk2-sources-3925

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

comment:8 Changed 5 years ago by therve

  • Keywords review added
  • Owner glyph deleted

comment:9 Changed 5 years ago by jkakar

  • Keywords review removed
  • Owner set to therve

[1]

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 5 years ago by exarkun

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

  • Resolution set to fixed
  • Status changed from new to closed

(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 4 years ago by <automation>

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