Code size report:

Reference:  shared/runtime/pyexec: Don't execute empty line in event driven REPL. [2e3304a]
Comparison: tests/extmod/ssl_poll: Test TLS transport cleanup. [merge of 58aae4b]
  mpy-cross:    +0 +0.000% 
   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:  +144 +0.017% standard
      stm32:    +0 +0.000% PYBV10
      esp32:   +48 +0.003% ESP32_GENERIC
     mimxrt:    +0 +0.000% TEENSY40
        rp2:   +32 +0.003% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

My understanding of the regression is that the new clean-up behavior closes the socket sooner than the peer/client can react to the error we attempt to flush before it.

The one trade-off/alternative I ruled out in the initial PR as a bad-practice trade off was removing the call that deletes the reference to the socket - but not cleaning up either. That makes MicroPython (and the application running on it, say Microdot) responsible for calling close() on the socket.

That would create a short delay between sending the error to the peer and tearing down the socket - which might avoid the regression. But it isn't a guaranteed fix, and if the processing is slowed down for any reason the same regression manifests. And I'm not sure that making Python code responsible for socket clean-up like this is appropriate - it's fragile and does what I deem to be the job of C-level code.

I'm unsure what the right solution from here is. The bug that would be fixed by the PR in its current state seems more severe to me than the regression it might introduce, but neither is ideal.

I think that the decision for the best course of action from this point is better fit for the core/active maintainers of this project. There also might be a better solution which avoids both problems that I don't see due to insufficient familiarity with this project.

@projectgus yeah.. :/

I'm unsure what is appropriate for micropython. In order for the correct error to reach the peer, I'm unsure if there is any different approach other than delaying the clean up someway - which would mean that socket exhaustion can still happen while waiting.

Maybe I am missing something, but I haven't been able to think up a solution that avoids this peer-received error regression and the socket exhaustion the current upstream has.

In the project where we encountered the bug, we decided to go with this fix accepting this regression, since the socket exhaustion was actively biting us and making SSL unworkable, whereas wrong errors were simply mildly annoying in comparison. I'm in no position to say which is better for the main micropython repository though.

I had a look at the root cause, and it seems like it might be an mbedTLS bug when performing the handshake asynchronously. Here's the packet capture from one of my tests:

• 16 - client sends certificate
• 17 - server sends fatal alert
• 18 - server sends TCP FIN
• 19 - client sends a client key exchange message
• 20 - server responds with RST as the socket is closed

From the mbedTLS side, this looks like the client's call to mbedtls_ssl_write() failing with 32 EPIPE (packets 19 & 20 above). If mbedTLS was to read from the socket at this point then it would receive the pending alert sent by the server step 17, but MicroPython doesn't call mbedtls_ssl_read() and instead raises the EPIPE back as an error.

If I patch it to call mbedtls_ssl_read() before raising EPIPE then this also doesn't work because this calls through to ssl_prepare_handshake_step() which calls mbedtls_ssl_flush_output() which (I think) tries to re-send the same unsent data from step 19, and this fails with EPIPE again...

So that kind of feels like an mbedTLS bug, that it's possible for it to swallow an alert received during the handshake because the handshake step logic (which is internal to mbedTLS) keeps trying to write to the closed socket instead of reading the alert from it.

We're currently on mbedTLS v3.6.6, but in this future if we update to mbedTLS v4.x and this problem still appears then I might try raising it as an issue upstream.

That would make sense, though I'm not that deep into mbedTLS, so I can't confidently comment on it without first digging through it for some time on my own.

Where does that leave this PR? The regression is annoying, but the problem with letting sockets hang up to the point of exhaustion seems more severe to me, both from a usability standpoint and from the other...

Though as I've said, if the regression is meaningfully blocking from micropython maintainers' point of view I understand too.