#7798 Handle non-socket.error in TCP client and invalid port numbers.

[adiroiban]

Scope

Fixes #7798

See error example https://gist.github.com/adiroiban/3967df30aca9add2eb9a

This fixes an unhandled error when failing to connect (or listen) due to various reasons.

The previous review is at https://twistedmatrix.com/trac/ticket/7798#comment:19

Change

Always call the code from a deferred.
Add tests and update the handling when an invalid port range is provided.

Contributor Checklist

• There is an associated ticket in Trac. https://twistedmatrix.com/trac/ticket/7798
• The changes pass minimal style checks (see: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#GettingYourPatchAccepted )
• I have created a newsfragment in src/twisted/newsfragments/
• I have updated the automated tests.

How to test

See https://gist.github.com/adiroiban/3967df30aca9add2eb9a

[adiroiban]

@markrwilliams thanks for the update

These tests are gonna be ugly as for now Twisted just forwards the OS error, and it looks like each OS has a different error.

My approach was not not reivent the wheel and add PORT number validation in twisted, but just forward any number to the OS call.

Maybe this is not a good idea, and in twisted we can add a simple check for port number and raise a standard error. This would make a lot of tests nicer to read.

The tests are still nice as they document the low level error for each os...but I think there is not much use in them.

In Ubuntu 18.04 we now have

- ['getsockaddrarg: port must be 0-65535.']
+ [-8]

[glyph]

@adiroiban are you interested in resolving these conflicts and finishing up this PR? We should probably close it if it's dead

[adiroiban]

The error assertion in the test is ugly, as Twisted is just raising the low-level error.

the important part of this PR is to make sure that the errback is called on errors.

this should be ready for review

needs-review

---

There was an unrelated flaky test on macos ... but no more of the old cfreactor errors :)

[FAIL]
Traceback (most recent call last):
  File "/Users/runner/work/twisted/twisted/.tox/macos-withcov-alldeps/lib/python3.12/site-packages/twisted/internet/test/test_process.py", line 523, in test_errorDuringExec
    self.assertIn("\N{SNOWMAN}".encode(), output.getvalue())
  File "/Users/runner/work/twisted/twisted/.tox/macos-withcov-alldeps/lib/python3.12/site-packages/twisted/trial/_synctest.py", line 509, in assertIn
    raise self.failureException(msg or f"{containee!r} not in {container!r}")
twisted.trial.unittest.FailTest: b'\xe2\x98\x83' not in b''

twisted.internet.test.test_process.PTYProcessTestsBuilder_AsyncioSelectorReactorTests.test_errorDuringExec

[glyph]

There was an unrelated flaky test on macos ... but no more of the old cfreactor errors :)

Hmm. This might be #8840 still being an issue

[glyph]

I'd really like to see some code that confirms the assumptions as they are stipulated, since they should be pretty deterministic (we can pass an invalid port number and watch what happens), and to fix whatever is going on with the coverage reporting. Why aren't these tests being run?

[glyph]

… why is this never covered? It seems like this line should definitely see coverage

[adiroiban]

This line is covered.

The line 133 is not covered, as we only run the -nodeps- tests on Linux.

[adiroiban]

Thanks for checking this.

I'd really like to see some code that confirms the assumptions as they are stipulated, since they should be pretty deterministic (we can pass an invalid port number and watch what happens), and to fix whatever is going on with the coverage reporting. Why aren't these tests being run?

I am not sure what you are asking here.

The test are checking the actual low-level OS API.
There is no mock involved in these tests.

---

Note that the Linux test take about 4 minutes, while the Windows test take 7 minutes

This is why, you can see the coverage reported sooner... after only the Linux tests are executed.

[glyph]

I am not sure what you are asking here.

The test are checking the actual low-level OS API. There is no mock involved in these tests.

The comment makes a claim — that macOS will error in a particular way with erroneous integer port numbers. It would be nice to have a test which does exactly that, resolve an invalid port number, and see if it gets truncated or translated in the way it claims; not to test Twisted, but to test the assumptions upon which the implementation is based.

Not a hard blocker, would just be nice to have.

[glyph]

OK, looks like we have another #8840 failure, I'm going to open a revert.

[adiroiban]

Thanks @glyph for the extra info.

Now it makes sense.

---

I will write a separate check to make sure this will continue to fail on macOS

This code was written a long time ago with support for python 2.6

I now see that Linux is also failing

macos

% python3 
Python 3.9.6 (default, Nov 10 2023, 13:38:27) 
[Clang 15.0.0 (clang-1500.1.0.2.5)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import socket
>>> _AI_NUMERICSERV = getattr(socket, "AI_NUMERICSERV", 0)
>>> socket.getaddrinfo('127.0.0.1', -1, 0, 0, 0, _AI_NUMERICSERV)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/socket.py", line 953, in getaddrinfo
    for res in _socket.getaddrinfo(host, port, family, type, proto, flags):
socket.gaierror: [Errno 8] nodename nor servname provided, or not known
>>> 

linux

$ python3
Python 3.11.9 (main, Apr  6 2024, 17:59:24) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import socket
>>> _AI_NUMERICSERV = getattr(socket, "AI_NUMERICSERV", 0)
>>> socket.getaddrinfo('127.0.0.1', -1, 0, 0, 0, _AI_NUMERICSERV)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.11/socket.py", line 962, in getaddrinfo
    for res in _socket.getaddrinfo(host, port, family, type, proto, flags):
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
socket.gaierror: [Errno -8] Servname not supported for ai_socktype
>>> 

---

On Windows, no exception is raised

> python.exe
Python 3.11.7 (tags/v3.11.7:fa7a6f2, Dec  4 2023, 19:24:49) [MSC v.1937 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import socket
>>> _AI_NUMERICSERV = getattr(socket, "AI_NUMERICSERV", 0)
>>> socket.getaddrinfo('127.0.0.1', -1, 0, 0, 0, _AI_NUMERICSERV)
[(<AddressFamily.AF_INET: 2>, 0, 0, '', ('127.0.0.1', 65535))]

[glyph]

I now see that Linux is also failing

this kind of detail is exactly why i want to have a mechanical record of the behavior, rather than just a comment :-). Thanks for checking!

[adiroiban]

Done. Let me know if this is what you were expecting.

I am not used to write this type of tests... as this is a test for a stdlib function, not our code.

We have tests for _resolveIPv6 with invalid port number, and for me that is all that we should care about :)

[adiroiban]

I am going to write a test for socket.getaddrinfo to check that an exception is raised on Linux and macOS

[adiroiban]

This is ready for review. Thanks!