SocketPool.Socket.__hash__() is not immutable

[dhalbert]

__hash__() was added to Socket so a Socket could be put in a dict, for use in the adafruit_requests library. However, the hash used is not immutable, and changes when the socket is closed.

The hash returned could be id(), to make it immutable.. However, it might be worth looking at whether the use of Socket in adafruit_requests really needs a dictionary. I see two uses: ._open_sockets and ._socket_free. I'll bring this up in an issue in that repo.

Tagging @hierophect on this, who was trying to debug #4061, which may be caused by this. (SSLSocket has the same hash.)

[hierophect]

@dhalbert as a follow up to our conversation, just noting that returning (mp_uint_t)self for common_hal_ssl_sslsocket_get_hash does solve the keys issue in #4061, as predicted (I don't know if that's how I'm supposed to code it though, can I use id in the shared-bindings layer?). You mentioned you were concerned about other instances where the ID could change unexpectedly, making the hash mutable, were you able to find those issues?

[dhalbert]

The only other issue is this: #2687, which I think is not applicable. You could try the fix above, and see how it works.

You can can return mp_obj_id() to get the id value for the hash. I would think it's OK to do that in shared-bindings, because calling that function is kind of like calling a common-hal function.

[dhalbert]

In CPython sockets, socket.fileno() will return -1 if the socket is closed. So we could implement that (just return .num).

[hierophect]

@dhalbert you mean, keep the hash as returning id() so it isn't mutable, but implement socket.fileno() so we can still access the socket number? I think fileno was on my list anyway, along some of the other "filler" functions in socket.

[dhalbert]

Yes, keep using id(). But if we implement .fileno(), can we take advantage of the fact that it returns -1 when the socket is closed, rather than keeping local state about that? It's really a question about what the requests library can do with it.

[hierophect]

I tried using hash() on Cpython's sockets, and it seems to work fine. It's not related to the result of fileno of course, as that's also mutable in Cpython. I can't find any takes online about whether it's advisable to use Sockets as keys, though.

[hierophect]

@dhalbert if you mean internal state within the Requests library, I think so, yes. We create a sock->num in C when the socket is created, and set it to -1 when closed, and that's all that fileno would be returning. The only potential issue is that TLS sockets don't strictly need to have the original socket open to keep working - in Scott's original implementation, they didn't call the lwip_socket function at all, which is what allocates the file descriptor. But my implementation calls the LWIP initializer and doesn't close it, because it's the only way to give SSLSocket access to bind/listen/accept (it's based off the CPython implementation that does the same thing). So fileno() = -1 should basically always be an indicator that the socket has been closed.

[tannewt]

@hierophect Can you open the second LWIP socket only when you do the server APIs?