After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 766396 - io_add_watch is broken for sockets on Windows
io_add_watch is broken for sockets on Windows
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: gobject
Git master
Other Linux
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2016-05-13 19:51 UTC by Lukas
Modified: 2016-09-03 16:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patch (1.27 KB, patch)
2016-05-13 19:51 UTC, Lukas
committed Details | Review

Description Lukas 2016-05-13 19:51:01 UTC
Created attachment 327824 [details] [review]
proposed patch

It's possible to pass file descriptors and file-like objects (like sockets) to io_add_watch. Unfortunately the code in the overrides [1] creates unix channels. On Windows, this leads to a socket fd being used as a file fd which eventually segfaults python. When passing an int, it's up to the user what he passes, so we can't check whether it's an file or socket fd. But when the user passes a socket we can.

See the attachment for a proposed patch that checks whether the user passed a socket and uses win32_new_socket if appropriate.

Test: https://gist.github.com/carrotIndustries/aefd4552745096ac47ce4f920b0958e1

[1] https://git.gnome.org/browse/pygobject/tree/gi/overrides/GLib.py#n726
Comment 1 Christoph Reiter (lazka) 2016-06-02 09:09:55 UTC
Review of attachment 327824 [details] [review]:

Thanks. Looks reasonable to me.

(Any objections from anyone else?)
Comment 2 Lukas 2016-06-29 14:03:29 UTC
Since no one objected how about merging this patch?
Comment 3 Christoph Reiter (lazka) 2016-06-29 15:04:05 UTC
Review of attachment 327824 [details] [review]:

lgtm