Opened 15 years ago

Closed 14 years ago

#2425 defect closed fixed (fixed)

t.i.tcp should not use os.strerror for Windows socket error codes

Reported by: ghazel Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description

os.strerror() is not appropriate for Windows socket error codes.

os.strerror(10061) #=> 'Unknown error'

There is a different native function (FormatMessage) to get the real error message. However any method of calling that would introduce a dependency (win32api, ctypes, take your pick). In the interest of avoiding that, several of the common WSA error codes are in the socket module in socket.errorTab. It's not documented, but it's been around for almost 6 years: http://sourceforge.net/tracker/?func=detail&atid=105470&aid=406642&group_id=5470 And hey look, moshez.

So something like this would work better:

def strerror(err):
    errorTab = getattr(socket, "errorTab", {})
    if err in errorTab:
        return errorTab[err]
    return os.strerror(err)

strerror(10061) #=> 'Connection refused.'

The question is, where should it go? It almost seems like a t.python thing, but only t.i.tcp uses it in this way.

Attachments (3)

formatError.py (1020 bytes) - added by arkanes 15 years ago.
proper error code lookup on Windows
formatError.patch (5.4 KB) - added by arkanes 15 years ago.
patch adds formatError, unit tests
formatError2.patch (5.7 KB) - added by ghazel 14 years ago.
formatError patch, take 2

Download all attachments as: .zip

Change History (25)

comment:1 Changed 15 years ago by arkanes

The *real* fix is that os.strerror needs to use FormatMessage on Windows. In the meantime, I'm attaching a function that can be used to look up error codes through with fallback through win32, ctypes, the errorTab in socket module, and finally os.strerror.

I'm not sure what kind of unit tests would be appropriate for this - it's hard to test conditional module imports.

Changed 15 years ago by arkanes

Attachment: formatError.py added

proper error code lookup on Windows

comment:2 Changed 15 years ago by arkanes

Keywords: review added

comment:3 Changed 15 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to arkanes

I'm not the windows maintainer, so I can't really do a full review here, but I can tell you that the fact that there are no unit tests will be an instant reject when someone does do a thorough review, so you should probably add some.

comment:4 in reply to:  description Changed 15 years ago by ghazel

This is much nicer than the original suggestion. Thanks arkanes.

comment:5 Changed 15 years ago by arkanes

Keywords: review added

Added a couple unit tests to make sure the lookup works and that it doesn't bomb out on other platforms.

comment:6 Changed 15 years ago by Glyph

Keywords: review removed

Rather than having a conditional inside the test case, why not have two separate test cases, one for win32 platforms with the fallback and one without? Skip them as appropriate.

Sorry to bug you about trivia, but:

  • Each test must have a docstring, explaining what it is that it tests.
  • The convention for test case names is actually test_fooBarBaz, not testFooBarBaz. Unfortunately a large number of tests in Twisted haven't had their names cleaned up yet, but that is the official standard.

comment:7 in reply to:  6 Changed 15 years ago by arkanes

Replying to glyph:

Rather than having a conditional inside the test case, why not have two separate test cases, one for win32 platforms with the fallback and one without? Skip them as appropriate.

Sorry to bug you about trivia, but:

  • Each test must have a docstring, explaining what it is that it tests.
  • The convention for test case names is actually test_fooBarBaz, not testFooBarBaz. Unfortunately a large number of tests in Twisted haven't had their names cleaned up yet, but that is the official standard.

Patch updated - comments in tests moved to docstrings, docstrings on all tests, naming convention updated. Changed the first test to two tests with individual skips as you suggested.

comment:8 Changed 15 years ago by arkanes

Keywords: review added

Changed 15 years ago by arkanes

Attachment: formatError.patch added

patch adds formatError, unit tests

comment:9 Changed 15 years ago by arkanes

Misread naming convention, patch updated.

comment:10 Changed 15 years ago by Jean-Paul Calderone

Owner: arkanes deleted

comment:11 Changed 15 years ago by therve

Keywords: review removed
Owner: set to arkanes
Priority: normalhighest

Quick review:

  • Please use try: import; except ImportError:...; else: action instead of try: import; action; except ImportError...
  • the skip use the wrong case of methods (test_FormatMessageSafeToCall.skip instead of test_formatMessageSafeToCall.skip)
  • Update copyright of the 3 files touched.
  • Use from twisted.python import win32 instead of import twisted.python.win32 as win32
  • The tests are a bit disturbing... The first is run on non-win32, whereas the test file is test_win32. The second one is not really useful. And the third one makes actually the same calls as the original function. For example if formatError returns None, your tests will pass. I don't have a answer though, so it could probably go like this.

Changed 14 years ago by ghazel

Attachment: formatError2.patch added

formatError patch, take 2

comment:12 Changed 14 years ago by ghazel

Keywords: review added
Owner: changed from arkanes to therve
Priority: highestnormal

formatError2.patch addresses the review issues mentioned by therve. It's been 7 months, so I figured I'd take a crack at getting this finished.

comment:13 Changed 14 years ago by therve

(In [21460]) Apply tracker's patch with some fixes.

Refs #2425

comment:14 Changed 14 years ago by therve

Owner: therve deleted
Priority: normalhighest

I got my hands dirty by creating the branch win32-strerror-2425.

comment:15 Changed 14 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jean-Paul Calderone
Status: newassigned

formatError needs more tests. I'm not exactly sure what they'll look like yet. I'll try to write them.

comment:16 Changed 14 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: assignednew

Okay I did something in that branch. I have no Windows environment to test with, and all the Windows slaves are offline right now, so I'm done with this for now. Maybe it works, maybe not, someone else can figure it out.

comment:17 Changed 14 years ago by ghazel

Tests pass after exarkun's changes, and the function seems to still work too.

comment:18 Changed 14 years ago by Jean-Paul Calderone

thanks for checking this over ghazel.

comment:19 Changed 14 years ago by therve

Owner: set to Jean-Paul Calderone

OK, I merged forward to win32-strerror-2425-2, buildbot is green, please merge.

comment:20 Changed 14 years ago by therve

Keywords: review removed

comment:21 Changed 14 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [21682]) Merge win32-strerror-2425-2

Author: arkanes, ghazel, exarkun Reviewer: therve Fixes #2425

Change twisted.internet.tcp to use Windows-aware error code lookup when creating exceptions for connection failure.

comment:22 Changed 11 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.