8000 unix/modsocket: Accept a host+port array for socket.connect. by agatti · Pull Request #17289 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

unix/modsocket: Accept a host+port array for socket.connect. #17289

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

unix/modsocket: Accept a host+port array for socket.connect. #17289

wants to merge 1 commit into from
+163 −1

Conversation

agatti
Copy link
Contributor
@agatti agatti commented May 12, 2025 •
edited
Loading

Summary

This PR lets socket.connect accept an array containing a hostname string and a port number as the address to connect to, if the socket family is either AF_INET or AF_INET6. The existing behaviour of accepting a serialised sockaddr structure as the connection address is still present, maintaining compatibility with existing code.

This brings the behaviour of socket.connect almost in line with the other embedded ports' versions that use LWIP, and somewhat with CPython as well. The differences are as follows:

  • Addresses in numeric form are not supported: this is not supported in LWIP either but CPython can handle them
  • Connection argument can be either a two-elements list or tuple: this matches LWIP's implementation, but CPython can only accept a tuple for AF_INET/AF_INET6
  • A regular hostname can be passed as the address: this is not supported in LWIP (dotted quads only, see netutils_parse_inet_addr in shared/netutils/netutils.c as invoked by extmod/modsocket.c), but is supported in CPython.

Hostname resolution is performed internally via a call to socket.getaddrinfo, and in case of multiple compatible entries mapped to a single hostname the first one returned by getaddrinfo(2) will be used.

Testing

This has been tested manually as such (numeric addresses are what www.google.com resolves to me from here):

import socket

data = {
    socket.AF_INET: ["www.google.com", "64.233.184.99"],
    socket.AF_INET6: ["www.google.com", "2a00:1450:400c:c0b::69"],
}

for k, v in data.items():
    sf = (k,)
    for addr in v:
        s = socket.socket(*sf)
        s.connect((addr, 80))
        s.write("GET /\n\n")
        if len(s.recv(100)) != 100:
            raise Exception
        print(sf, addr, ": SUCCESS [STRING]")
        s.close()
        s = socket.socket(*sf)
        a = socket.getaddrinfo(addr, 80, k)[0][-1]
        s.connect(a)
        s.write("GET /\n\n")
        if len(s.recv(100)) != 100:
            raise Exception
        print(sf, addr, ": SUCCESS [SOCKADDR]")
        s.close()

with the output:

(2,) www.google.com : SUCCESS [STRING]
(2,) www.google.com : SUCCESS [SOCKADDR]
(2,) 64.233.184.99 : SUCCESS [STRING]
(2,) 64.233.184.99 : SUCCESS [SOCKADDR]
(10,) www.google.com : SUCCESS [STRING]
(10,) www.google.com : SUCCESS [SOCKADDR]
(10,) 2a00:1450:400c:c0b::69 : SUCCESS [STRING]
(10,) 2a00:1450:400c:c0b::69 : SUCCESS [SOCKADDR]

I see extmod/ssl_basic.py uses test.example.com for its tests - if a test file is required I may use that host myself as well.

Edit: a test file with ipv4/ipv6 connection tests using both host+port pair and a pre-resolved sockaddr buffer have been added.

Trade-offs and Alternatives

There is a slight footprint increase for the Unix port but, considering the average computing power and storage facilities of the average environment the Unix port runs in, the quality of life improvements, and increased compatibility should be enough benefits to make these code changes useful.

@codecov Codecov
Copy link
codecov bot commented May 12, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.54%. Comparing base (a05766f) to head (354d67a).
Report is 66 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #17289   +/-   ##
=======================================
  Coverage   98.54%   98.54%           
=======================================
  Files         169      169           
  Lines       21896    21897    +1     
=======================================
+ Hits        21578    21579    +1     
  Misses        318      318

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions GitHub Actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:  +398 +0.047% standard[incl +8(data)]
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@agatti agatti force-pushed the unix-socket-connect-host-port branch from dcdd59a to 0223a78 Compare May 19, 2025 00:53
@agatti
Copy link
Contributor Author
agatti commented May 19, 2025 •
edited
Loading

Are IPv6 connections blocked in the CI environment? Unless I'm missing something the IPv6 tests should succeed (they do from my LAN at least).

Edit: Indeed they are: actions/runner-images#668 :( I'll rewrite the tests to acknowledge this fact.

@agatti agatti force-pushed the unix-socket-connect-host-port branch from 0223a78 to c02c290 Compare May 19, 2025 01:23
@agatti
Copy link
Contributor Author
agatti commented May 19, 2025 •
edited
Loading

Interesting, socket.send(str) works on Unix - although it shouldn't. Wonder if that behaviour should be deprecated in v2.

@agatti agatti force-pushed the unix-socket-connect-host-port branch 3 times, most recently from fe48e57 to 32e83f1 Compare May 19, 2025 03:38
@agatti
unix/modsocket: Accept a host+port array for socket.connect.
354d67a 
This commit lets socket.connect accept an array containing a hostname
string and a port number as the address to connect to, if the socket
family is either AF_INET or AF_INET6.

This brings the behaviour of socket.connect in line with the other
embedded ports' versions that use LWIP, and also with CPython - even
though CPython can only accept a tuple as its host+port argument.  The
existing behaviour of accepting a serialised sockaddr structure as the
connection address is still present, maintaining compatibility with
existing code.

Signed-off-by: Alessandro Gatti <a.gatti@frob.it>
@agatti agatti force-pushed the unix-socket-connect-host-port branch from 32e83f1 to 354d67a Compare May 19, 2025 04:29
@dpgeorge
Copy link
Member

Thanks for the patch.

There's a bit of history here why this isn't supported. And it's documented: https://docs.micropython.org/en/latest/library/socket.html#socket-address-format-s . In particular, quoting that page:

  • Always use getaddrinfo when writing portable applications.
  • Tuple addresses described below can be used as a shortcut for quick hacks and interactive use, if your port supports them.

You'll see there's actually a socket module in micropython-lib/unix-ffi which adds this functionality. And it adds it for accept, bind, connect, sendall and sendto.

So that's some background... not sure how to proceed with this PR at this stage.

@agatti
Copy link
Contributor Author
agatti commented May 27, 2025

Thanks for the explanation! I've never really used ffi nor micropython-lib when dealing with the Unix port, I usually develop directly on device myself and use Unix just to validate some things that would take longer to test elsewhere.

From what I've seen (and I hope I didn't miss anything) there are only two ports that have sockets support and can't natively handle a host+port tuple in socket.connect: Unix and zephyr. Some ports that can handle tuples are limited to dotted quads, whilst others call an internal implementation of getaddrinfo.

So, I wonder whether adding at least dotted quad support to Zephyr (and maybe to Unix without ffi to make it easier for interactive use) would make sense at this point, to actually make things more compatible across ports for simpler cases.

If that's not something that can help, no worries, I'll close the PR :) On the other hand, maybe the sockaddr-only unit test cases can still be saved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
port-unix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@agatti @dpgeorge
0