Skip to content
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.

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

Separate SSLSocket from Socket #4049

Merged
merged 5 commits into from
Feb 2, 2021
Merged

Separate SSLSocket from Socket #4049

merged 5 commits into from
Feb 2, 2021

Conversation

hierophect
Copy link
Collaborator

@hierophect hierophect commented Jan 22, 2021

This PR removes the TLS implementations from Socket and moves them to a new object class, SSLSocket, which is created from a Socket object using SSL's contex.wrap_socket() function. This behavior more closely matches that of Cpython.

Opening as a draft as I've had a couple difficulties with this change and I'm still testing:

  • The regular Socket class has a new LWIP-only implementation for connect, but I've had issues making this non-blocking. A non-blocking call to connect should always return a -1 "EINPROGRESS" error for the first call, but I don't find that they ever connect, no matter how long the timeout is. The current implementation simply switches the entire socket to blocking mode briefly, but other failed implementations are shown as comments.
  • Still trying to figure out exactly how SSLSocket implements bind and listen and accept - as best I can tell, they should call the original functions (ie LWIP ones) and then just wrap whatever socket gets accepted as a SSLSocket. But I'm still not sure about this.
  • I don't have a great SSL python example so if anyone has a demo they like that'd be good.

Also includes bugfixes for a variety of user-reported issues with Socket.
Resolves #4057
Resolves #4101

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skimmed it and only see one thing to change. Thanks for following up with this!

ports/esp32s2/common-hal/ssl/SSLContext.c Outdated Show resolved Hide resolved
@hierophect
Copy link
Collaborator Author

It should also go without saying that this PR breaks the heck out of requests

@tannewt
Copy link
Member

tannewt commented Jan 26, 2021

It should also go without saying that this PR breaks the heck out of requests

Why? It shouldn't because this is the same API as CPython which requests works with.

@hierophect
Copy link
Collaborator Author

hierophect commented Jan 26, 2021

Why? It shouldn't because this is the same API as CPython which requests works with.

Yes, sorry, you're right. There was an error in wrap_socket that I confused for being an incompatibility. Wrap_socket now accepts sockets with a valid LWIP number since they're used for the bind-listen-accept methods.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Thank you for separating these out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SocketPool.Socket.__hash__() is not immutable ESP32-S2 "Out of sockets" in 6.2.0-beta.0
2 participants