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

Correctly raise OS error in socketpool_socket_recv_into() #7623

Merged
merged 1 commit into from
Feb 21, 2023

Conversation

DavePutz
Copy link
Collaborator

Fixes #7606. Instead of returning an errno (which seems to lose the negative sign along the way and so will be interpreted as a received count) instead raise an OSerror with the errno in socketpool_socket_recv_into(). Tested using TLS with a local mosquitto server and io.adafruit.com.

@dhalbert dhalbert changed the base branch from main to 8.0.x February 21, 2023 20:01
@dhalbert dhalbert changed the base branch from 8.0.x to main February 21, 2023 20:01
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

This makes complete sense to me; thank you very much @DavePutz. I will backport this to 8.0.x after it is merged.

@jepler
Copy link
Member

jepler commented Feb 22, 2023

I think this needs further examination, as the routine where the exception is now raised is used in the web workflow and exceptions are not allowed to be raised in that code. I'll work on an alternate fix today.

jepler added a commit to jepler/circuitpython that referenced this pull request Mar 22, 2023
This reverts commit 7e6e824.

Fixes adafruit#7770

The change in adafruit#7623 needs to be revered; the raise-site added in adafruit#7632
is the correct one and the one in socketpool needs to be reverted.

This is not affecting 8.0.x because adafruit#7623 was not back-ported to there
before we realized it was not a full fix.

Both adafruit#7770 and adafruit#7606 should be re-tested. I didn't test.
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.

minimqtt lib problem with TLS broker
3 participants