Broken out from bug 7636. See that bug for details. Port open check is used to check for open ports on the agent, and uses asyncore to do so asynchronously. We want to convert the code to only use asyncio instead.
The old way to manage exceptions from CheckTCPPortOpen was to handle the exceptions using handle_error() from inside the class, which would print an error message + traceback and then return the status SOCKET_OPEN_ERROR. This meant that from the outside, the caller would not be aware of the exception and just look at the status to decide what to do. There are two ways of managing exceptions in the new asyncio code to match this behaviour. 1. We let portopencheck.py do the error handling by catching exceptions (similarly to before), and logging them in the same way that handle_error() would. It would also set the status to SOCKET_OPEN_ERROR. From the outside, the caller would not need to handle any Exceptions at all and would just have to look at the socket_status. 2. We just pass on the exception to the future inside portopencheck.py. This would mean that the functionality of the old handle_error() would have to be moved to two places in handler_reqsession in the new code. I think it's best to go with option 1; we completely mask exceptions from the caller and use the returned status to decide what to do instead. This is more similar to how the old code worked and would mean less code changes overall.
The module is now completely converted from asyncore to asyncio. MUST: * The port open check code no longer relies on asyncore. ✓ SHOULD: * Everything uses asyncio terminology rather than asyncore ✓
Unit testing: ✅ All tests pass. Manual testing: In order to be able to investigate the branch coverage surrounding the usage of the two new functions, portopencheck.check_unix_port_open() and portopencheck.check_tcp_port_open(), I added temporary logging in the branches I want to reach. ############### check_unix_port_open() ############### I first looked at check_unix_port_open(), which is called if the handler finds a socket file that is not recognized by ThinLinc. Given that this occurs, there are four branches we need to look at. In all relevant branches, the socket file found is always named /tmp/.X11-unix/X10 Branch 1 - If nothing is connected to the socket file, we use it for the session: 1. Create a socket file by running "nc -Ul /tmp/.X11-unix/X10". 2. Exit nc and verify that the X10 socket file is still exists. 3. Create a new session with tlclient. > ✅ Result 1: The socket named X10 becomes associated with the session. Branch 2: If something is connected to the socket file, we use next available display number: 1. Create a socket file by running "nc -Ul /tmp/.X11-unix/X10". 2. Do *not* exit nc and verify that the socket file exists. 3. Create a new session with tlclient. > ✅ Result 2: The next available display number 11 is used, and the socket > named X11 becomes associated with the session. Branch 3: If check_unix_port_open() reaches a timeout, we use the socket file for the session. 1. Create a socket file by running "nc -Ul /tmp/.X11-unix/X10". 2. Do *not* exit nc and verify that the socket file exists. 3. Make the call to check_unix_port_open() timeout, using e.g. asyncio.sleep() 4. Create a new session with tlclient. > ✅ Result 3: After a timeout, the socket named X10 becomes associated with > the session. Branch 4: If some OS related error other than "Connection refused" is raised, log and return failure status to the client: 1. Raise an OSError other than "Connection refused" in appropriate location in check_unix_port_open(). 2. Create a new session with tlclient. > ✅ Result 4: A failure is returned to the client with the message > "ThinLinc login failed". ############### check_tcp_port_open() ################ Given that a display number and its corresponding unix socket has been selected, check_tcp_port_open() is used to wait for a matching TCP port to be available. The function will be called over and over until the port can be opened or until a timeout is reached. There are two branches to investigate, one where the port is eventually opened and one where it is not, and the timeout occurs. Branch 5: The TCP port eventually becomes available: 1. Create a new session with tlclient. > ✅ Result 5: A port matching the display number is eventually opened after > a few attempts with check_tcp_port_open(). Branch 6: The TCP port does not become available before a timeout, connection failure status is returned to the client: 1. Manually set the result of check_tcp_port_open() to false. 2. Create a new session with tlclient. > ✅ Result 6: A failure is returned to the client with the message > "ThinLinc login failed" after the timeout is reached. > ✅ All tests pass
MUST: * The port open check code no longer relies on asyncore. > ✅ The code is now using asycio instead of asyncore. SHOULD: * Everything uses asyncio terminology rather than asyncore > ✅ The new code uses asyncio terminology. ######################################## Even so, we need to reopen this bug due to a couple of reasons: 1. The updated code is not closing sockets upon successful connection. This is inconsistent with the old code, and should be done, since leaving the socket hanging could have unforeseen consequences (such as ports becoming unavailable for longer than expected). 2. check_tcp_port_open(addr) and check_unix_port_open(addr) do not actually share a function signature, where check_tcp_port_open(addr) requires an IP address and a port number, while check_unix_port_open(addr) requires a unix path. This should be updated to make the code more readable.
(In reply to Alexander Zeijlon from comment #13) > 1. The updated code is not closing sockets upon successful connection. The file has been updated so that the socket is closed upon successful connection. > 2. check_tcp_port_open(addr) and check_unix_port_open(addr) do not actually > share a function signature, where check_tcp_port_open(addr) requires an IP > address and a port number, while check_unix_port_open(addr) requires a unix > path. This should be updated to make the code more readable. The function signatures have been updated so that they match the signatures used by the asyncio functions that are called internally. Hopefully, this should make things more readable and reduce any confusion. I tested the latest build on Fedora 38 (Python 3.11.4) and got the same results as in comment #12.
All new updates look good. But I noticed that we don't have any test that covers closing of the sockets.
I have looked through the latest commits and run the tests for portopencheck.py, and everything looks good. Closing.
