-
Notifications
You must be signed in to change notification settings - Fork 204
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
Fail to upload long Blob using mbedtls+compact(Issue #1995) #536
Fail to upload long Blob using mbedtls+compact(Issue #1995) #536
Conversation
adapters/httpapi_compact.c
Outdated
@@ -235,7 +235,7 @@ HTTP_HANDLE HTTPAPI_CreateConnection(const char* hostName) | |||
tlsio_config.port = 443; | |||
tlsio_config.underlying_io_interface = NULL; | |||
tlsio_config.underlying_io_parameters = NULL; | |||
|
|||
tlsio_config.protocol="http"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @maallahatem , thank you very much for the PR.
I have a few points about the code change though.
- The TLS layer should not be tied to any specific protocol, so the best way would be create a flag in the tlsio_config structure like "invoke_on_send_complete_callback_for_fragments" (true/false);
- I am concerned about the behavior of the upper layer if it now starts firing multiple
on_send_complete
for a single send (single from an upper layer perspective) - I will need to test to make sure this is not a breaking change for the SDK; - I need to double check if something need to be done on the other tlsio_* adapters for this new flag, in case they also need to consume it or document that they ignore it completely.
For now if you could address item 1 above, I'll work on items 2 and 3 within the next week.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @maallahatem , I see you made the changes. We will verify them next.
@@ -1408,6 +1408,7 @@ HTTPAPI_RESULT HTTPAPI_SetOption(HTTP_HANDLE handle, const char* optionName, con | |||
tlsio_config.port = 443; | |||
tlsio_config.underlying_io_interface = http_proxy_io_get_interface_description(); | |||
tlsio_config.underlying_io_parameters = &proxy_config; | |||
tlsio_config.invoke_on_send_complete_callback_for_fragments=true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tlsio_config.invoke_on_send_complete_callback_for_fragments=true; | |
tlsio_config.invoke_on_send_complete_callback_for_fragments = true; |
{ | ||
// trigger callback always on failure, otherwise call it on last fragment completion | ||
// In case of http communication (ie blob upload), The callback is called with each fragment | ||
if((tls_io_instance->invoke_on_send_complete_callback_for_fragments && tls_io_instance->send_complete_info.is_fragmented_req)|| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if((tls_io_instance->invoke_on_send_complete_callback_for_fragments && tls_io_instance->send_complete_info.is_fragmented_req)|| | |
if ((tls_io_instance->invoke_on_send_complete_callback_for_fragments && tls_io_instance->send_complete_info.is_fragmented_req)|| |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Still unit test failures 2021-06-25T18:29:36.9899908Z ==3366== HEAP SUMMARY: |
I'm using Valgrind on my side and everything feels fine
|
We can't upgrade from valgrind 3.12 because there is no newer release on Ubuntu-18.04 I can look at adding valgrind suppressing rules. There are still other failures as below. 2021-06-25T18:29:35.4783949Z [0mExecuting test tlsio_mbedtls_dowork_handle_NULL_fail ... |
Humm, ran it under valgrind v3.15 and I still see the memory leak errors. Are you sure you are on the branch SE/fix_blob_upload_fail_mbedtls_1995?
|
I'm sorry, it seems that I'm using the wrong branch, Regards |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@maallahatem thanks. We will validate this before merging the PR. |
Hello,
The aim of this pull request is to propose a fix for the bug Fail to upload long Blob using mbedtls+compact #1995
Note, that the other part of the solution is available a PR on the azure-iot-sdk-c repository #2004
Regard
Hatem