Opened 12 years ago

Last modified 7 years ago

#1502 defect new

call selectreactor._preenDescriptors for WSAENOTSOCK

Reported by: titty Owned by: Corbin Simpson
Priority: normal Milestone:
Component: core Keywords: win32
Cc: titty, teratorn, zseil Branch:
Author:

Description


Attachments (1)

1502.patch (748 bytes) - added by Corbin Simpson 7 years ago.
Simple patch, with minimal profanity

Download all attachments as: .zip

Change History (15)

comment:1 Changed 12 years ago by titty

we have a win32 server doing perspective broker via ssl.

it started printing the following message again and again:

22.02.06 14:05:24 [-] Unexpected error in main loop.
22.02.06 14:05:24 [-] Failure: select.error (10038, 'Ein Vorgang bezog sich auf
ein Objekt, das kein Socket ist')
        Traceback (most recent call last):
          File "c:/ralf/bbot-2006-1/bb-dir/win32/bin/nbcore.py", line 6, in ?
            main()
          File "C:\ralf\bbot-2006-1\py-nextbot\nextbot\apps\nbcore.py", line
133, in main
            _main()
          File "C:\ralf\bbot-2006-1\py-nextbot\nextbot\apps\nbcore.py", line
126, in _main
            reactor.run()
          File
"c:\ralf\bbot-2006-1\extern\Twisted\twisted\internet\posixbase.py", line 206, in run
            self.mainLoop()
        --- <exception caught here> ---
          File
"c:\ralf\bbot-2006-1\extern\Twisted\twisted\internet\posixbase.py", line 217, in
mainLoop
            self.doIteration(t)
          File
"c:\ralf\bbot-2006-1\extern\Twisted\twisted\internet\selectreactor.py", line 97,
in doSelect
            [], timeout)
          File
"c:\ralf\bbot-2006-1\extern\Twisted\twisted\internet\selectreactor.py", line 49,
in win32select
            r, w, e = select.select(r, w, w, timeout)
        select.error: (10038, 'Ein Vorgang bezog sich auf ein Objekt, das kein
Socket ist')
        
=== twisted/internet/selectreactor.py
==================================================================
--- twisted/internet/selectreactor.py   (revision 22116)
+++ twisted/internet/selectreactor.py   (local)
@@ -117,6 +117,16 @@
                     return
                 elif se.args[0] == EBADF:
                     self._preenDescriptors()
+                elif se.args[0] == 10038:
+                    #
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winsock/winsock/windows_sockets_error_codes_2.asp
+                    # WSAENOTSOCK - 10038
+                    # Socket operation on nonsocket.
+                    # An operation was attempted on something that is
+                    # not a socket. Either the socket handle parameter
+                    # did not reference a valid socket, or for select,
+                    # a member of an fd_set was not valid. 
+                    log.msg("10038: FUCK WINDOWS")
+                    self._preenDescriptors()
                 else:
                     # OK, I really don't know what's going on.  Blow up.
                     raise

With this patch  get:

22.02.06 14:19:49 [-] 10038: FUCK WINDOWS
22.02.06 14:19:49 [-] Malformed file descriptor found.  Preening lists.
22.02.06 14:19:49 [-] bad descriptor <Broker #1 on 1512>
22.02.06 14:19:49 [-] bad descriptor <Broker #1 on 1512>

this is python 2.4.2 running on xp sp2.

comment:2 Changed 12 years ago by teratorn

Cc: teratorn added
Component: conch
Owner: set to z3p

Can you provide example code that reproduces the problem? If not, can you please give us as many related details as possible... Twisted version, conditions under which the problem occurs, etc.

comment:3 Changed 12 years ago by titty

No example code. The problem was actually caused by some memory being overwritten. Nevertheless that WSAENOTSOCK error should be caught.

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

Component: conchcore
Owner: changed from z3p to PenguinOfDoom

Why should we handle a case that can never legimately occur?

comment:5 Changed 12 years ago by titty

For the same reason EBADF is handled. This is just another case of "malformed filedescriptor", which you do handle in the EBADF case, and which you currently don't handle in the WSAENOTSOCK case.

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

Priority: highnormal

EBADF is raised if you select on a socket you've closed. While this doesn't really seem legitimate to me, that code has the advantage of already being in Twisted.

If the only way WSAENOTSOCK can be raised is if you've corrupted some memory, then it seems pretty pointless to handle it. There's an uncountable number of other places in Twisted which will fall over hard from random memory corruption.

However, if you add a unit test then I guess we can handle the case.

comment:7 Changed 12 years ago by titty

Resolution: wontfix
Status: newclosed

No, my local copy of twisted is handling that error. I don't care that much.

comment:8 Changed 11 years ago by titty

Resolution: wontfix
Status: closedreopened

reopening, as apparently ([18156]) closing sockets is enough to trigger that WSAENOTSOCK error (and it therefore should receive the same treatment as EBADF).

comment:9 Changed 7 years ago by <automation>

Owner: PenguinOfDoom deleted

Changed 7 years ago by Corbin Simpson

Attachment: 1502.patch added

Simple patch, with minimal profanity

comment:10 Changed 7 years ago by Corbin Simpson

Keywords: review added

Low-hanging fruit, delicious. Attached a patch for review.

Open questions:

  1. How to unit test this? There's no related test for EBADF which I can see nearby; help?
  2. Is there even any legitimate way to get a non-socket selectable into the reactor?

comment:11 in reply to:  10 Changed 7 years ago by zseil

Cc: zseil added
Keywords: review removed

Replying to MostAwesomeDude:

Low-hanging fruit, delicious. Attached a patch for review.

Open questions:

  1. How to unit test this? There's no related test for EBADF which I can see nearby; help?
  2. Is there even any legitimate way to get a non-socket selectable into the reactor?

Hello, thanks for working on this! Here is a quick review:

  1. Instead of adding the numerical constant as another error case, it would be better to from errno import WSAENOTSOCK as EBADF in the Windows specific block at the top of the module. According to MSDN select on Windows never returns with EBADF.
  2. Something similar should be done for EINTR, winsock's select returns WSAEINTR instead.
  3. It looks like the if se.args[0] in (0, 2): error case could be removed, the Windows specific select wrapper is handling that case now. But these last two points could probably be fixed in a separate ticket.
  4. To test your change, look at twisted.internet.test.test_fdset.ReactorFDSetTestsBuilder.test_add* tests. You could close the underlying socket in do* methods and check that the appropriate error got raised in connectionLost method.

comment:12 Changed 7 years ago by zseil

Owner: set to Corbin Simpson
Status: reopenednew

Ups, forgot to reassign the ticket back to you.

comment:13 Changed 7 years ago by Corbin Simpson

Before I progress further on this ticket, could I get some confirmation that errno actually contains those Win32-specific constants on Win32? I don't have a box available to test this with, myself, and I'm not sure if this whole shebang works in Wine. (I'm really not the best person for this ticket, but nobody else appears to care.)

comment:14 Changed 7 years ago by zseil

Those constants do exist in errno at least in Python 2.4 and newer versions, but I think it is likely that a ReactorBuilder based test will uncover similar bugs in other reactors. So I would suggest to test patch on buildbots before committing to trunk.

Note: See TracTickets for help on using tickets.