Ticket #3925 defect closed fixed

Opened 5 years ago

Last modified 4 years ago

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

1

Changed 4 years ago by therve

  • cc therve added

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

2

Changed 4 years ago by exarkun

Where did you see that?

3

Changed 4 years ago by therve

Sorry I got confused.

4

Changed 4 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);

5

Changed 4 years ago by therve

6

Changed 4 years ago by exarkun

This is blocking the resolution of #745.

7

Changed 4 years ago by therve

  • branch set to branches/gtk2-sources-3925
  • branch_author set to therve

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

8

Changed 4 years ago by therve

  • owner glyph deleted
  • keywords review added

9

Changed 4 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.

10

Changed 4 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

11

Changed 4 years ago by therve

  • status changed from new to closed
  • resolution set to fixed

(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.

12

Changed 3 years ago by <automation>

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