TLSSocket send/recv return WOULD_BLOCK error instead of NO_CONNECTION

[michalpasztamobica]

Description

In case mbedtls handshake advertising MBEDTLS_ERR_SSL_WANT_READ or MBEDTLS_ERR_SSL_WANT_WRITE is not really a critical error and should be recoverable. We improve the accurateness of the return value from send/recv function by returning NSAPI_ERROR_WOULD_BLOCK instead of NSAPI_ERROR_NO_CONNECTION in such case.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo
@kjbracey-arm

[ciarmcom]

@michalpasztamobica, thank you for your changes.
@SeppoTakalo @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

[SeppoTakalo]

I don't see why you are changing this?

When calling Socket::connect() in non blocking mode:

1. First call returns NSAPI_ERROR_IN_PROGRESS which means that asyncronous operation started.
2. All subsequent calls return NSAPI_ERROR_ALREADY until connection is reached.
3. When connected, returns NSAPI_ERROR_IS_CONNECTED

There is no WOULD_BLOCK in Socket::connect()

What was the problem you are fixing?

[SeppoTakalo]

This sounds like you are changing the API.

[michalpasztamobica]

@SeppoTakalo , thank you for your alertness, I overinterpreted my task. There should be no API change, indeed. The PR was only meant to address the return type of continue_handshake. Now I see that connect's behavior should remain unchanged. I amended the PR, I hope it's ok now.

[michalpasztamobica]

With the current code the status is following:

• continue_handshake returns NSAPI_ERROR_WOULD_BLOCK in case the WANT_READ or WANT_WRITE are returned from mbedtls_ssl_handshake().
• In connect() call this will result in NSAPI_ERROR_IN_PROGRESS returned, as was the case so far
• However the recv() and send() will now return NSAPI_ERROR_WOULD_BLOCK if they are in non-blocking mode, instead of NSAPI_ERROR_NO_CONNECTION. which was not exactly adequate in this context.

@SeppoTakalo , please confirm that we have the common understanding of the situation.
@kjbracey-arm , would you please share your thoughts on this?

[SeppoTakalo]

Not exactly,

continue_handshake() returns:

• NSAPI_ERROR_ALREADY if mbed TLS returned MBEDTLS_ERR_SSL_WANT_READ/WRITE
• NSAPI_ERROR_AUTH_FAILURE on any failure
• NSAPI_ERROR_IS_CONNECTED if connection was established.

This is called from connect(), which might translate calls, and return:

• NSAPI_ERROR_IN_PROGRESS in case this was the first call to connect() and continue_handshake() returned NSAPI_ERROR_ALREADY
• NSAPI_ERROR_OK in case this is first call, and continue_handshake() returned NSAPI_ERROR_IS_CONNECTED.

Same kind of translation might happen in send()/recv() calls, where they might call continue_handshake() in case TLS handshake was not completed. In those case it might return:

• NSAPI_ERROR_NO_CONNECTION if continue_handshake() returns NSAPI_ERROR_ALREADY. (This can be changed, probably, not have not been tested).
• NSAPI_ERROR_AUTH_FAILURE on any failure
  And in case of NSAPI_ERROR_IS_CONNECTED is returned, it continues to read/write.

[michalpasztamobica]

I have run an internal CI job against the mbed-client-testapp and all tests passed fine. @SeppoTakalo , do you have more ideas on what we could check before merging this into master?
It looks safe and reasonable to me :)

[SeppoTakalo]

Think this looks OK now

[michalpasztamobica]

@0xc0170 , this PR is already approved. Could you change the label, please?

[0xc0170]

CI started

[mbed-ci]

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts