Skip to content

extmod/modussl_mbedtls: Add closenotify() method #10783

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

Closed
wants to merge 1 commit into from

Conversation

dimkr
Copy link

@dimkr dimkr commented Feb 18, 2023

Sending a FIN without a close_notify alert breaks some protocols: in my case, Gemini. (I'm running a Gemini server on a Pico W.)

This can block or slow down existing code that expects .close() not to do this, so it's a separate method.

@dimkr dimkr force-pushed the feature/close-notify branch from 3c91cee to eaaa21f Compare February 18, 2023 17:12
@dimkr dimkr changed the title extmod/modussl_mbedtls: add closenotify() method extmod/modussl_mbedtls: Add closenotify() method Feb 18, 2023
Signed-off-by: Dima Krasner <dima@dimakrasner.com>
@dimkr dimkr force-pushed the feature/close-notify branch from eaaa21f to 6c7c0a1 Compare February 18, 2023 17:15
@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 

@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2023

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 98.48%. Comparing base (4598b89) to head (6c7c0a1).
Report is 2135 commits behind head on master.

Files with missing lines Patch % Lines
extmod/modussl_mbedtls.c 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10783      +/-   ##
==========================================
- Coverage   98.50%   98.48%   -0.02%     
==========================================
  Files         155      155              
  Lines       20540    20544       +4     
==========================================
  Hits        20232    20232              
- Misses        308      312       +4     

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

@dpgeorge dpgeorge added the extmod Relates to extmod/ directory in source label Feb 19, 2023
@dpgeorge
Copy link
Member

Thanks for the contribution.

This is not compatible with CPython, which doesn't have this closenotify() method. We need to find a solution that is compatible with CPython.

If you were implementing a Gemini server to run under CPython, how would it work?

@dimkr
Copy link
Author

dimkr commented Feb 20, 2023

I can move this to close(), requiring a CLOSING state before CLOSED. That would make close() a potentially blocking call.

EDIT: looked at cpython, close_notify belongs in shutdown().

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

@dpgeorge
Copy link
Member

Closing due to inactivity. If this is still of interest feel free to reopen with some more detailed discussion about how CPython handles this case. (Also note that MicroPython now has a tls module which can have incompatibilities with CPython but it would be best to try and make the functionality match CPython.)

@dpgeorge dpgeorge closed this Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extmod Relates to extmod/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants