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.

Sign up for GitHub

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

Unit test for removing callback severance on 5-to-3 adapter #553

Merged
merged 9 commits into from
Oct 25, 2023
Merged

Unit test for removing callback severance on 5-to-3 adapter #553

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
60d2b99
unit test for adapter operation callback
xiazhvera Oct 9, 2023
9f58358
remove unused var
xiazhvera Oct 9, 2023
3211d3c
fix compile error: remove unused vars
xiazhvera Oct 9, 2023
d35f854
update mqtt submodule
xiazhvera Oct 19, 2023
2ab1706
setup mqtt to test branch
xiazhvera Oct 20, 2023
fd76b4c
Merge branch 'main' of https://github.com/awslabs/aws-crt-cpp into ad…
xiazhvera Oct 23, 2023
1201e4c
update mqtt
xiazhvera Oct 23, 2023
c7f672b
Merge branch 'main' into adapter_test
xiazhvera Oct 24, 2023
c8093d0
Merge branch 'main' into adapter_test
xiazhvera Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ if(NOT BYO_CRYPTO)
add_net_test_case(Mqtt5to3AdapterWithIoTConnectionThroughMqtt5)
add_net_test_case(Mqtt5to3AdapterDirectConnectionWithMutualTLSThroughMqtt5)
add_net_test_case(Mqtt5to3AdapterOperations)
add_net_test_case(Mqtt5to3AdapterNullPubAck)
add_net_test_case(Mqtt5to3AdapterMultipleAdapters)
endif()

Expand Down
44 changes: 44 additions & 0 deletions tests/Mqtt5ClientTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3166,6 +3166,50 @@ static int s_TestMqtt5to3AdapterOperations(Aws::Crt::Allocator *allocator, void
}
AWS_TEST_CASE(Mqtt5to3AdapterOperations, s_TestMqtt5to3AdapterOperations)

/*
* [Mqtt5to3Adapter-UC11] Test s_TestMqtt5to3AdapterNullPubAck
* The unit test would have memory leak if the callback data for incomplete publish was not released.
*/
static int s_TestMqtt5to3AdapterNullPubAck(Aws::Crt::Allocator *allocator, void *)
{
Mqtt5TestEnvVars mqtt5TestVars(allocator, MQTT5CONNECT_IOT_CORE);
if (!mqtt5TestVars)
{
printf("Environment Variables are not set for the test, skip the test");
return AWS_OP_SKIP;
}

ApiHandle apiHandle(allocator);

Aws::Iot::Mqtt5ClientBuilder *builder = Aws::Iot::Mqtt5ClientBuilder::NewMqtt5ClientBuilderWithMtlsFromPath(
mqtt5TestVars.m_hostname_string,
mqtt5TestVars.m_certificate_path_string.c_str(),
mqtt5TestVars.m_private_key_path_string.c_str(),
allocator);
ASSERT_TRUE(builder);

String testUUID = Aws::Crt::UUID().ToString();
String testTopic = "test/MQTT5to3Adapter_" + testUUID;
ByteBuf testPayload = Aws::Crt::ByteBufFromCString("PUBLISH ME!");

std::shared_ptr<Aws::Crt::Mqtt5::Mqtt5Client> mqtt5Client = builder->Build();
ASSERT_TRUE(mqtt5Client);
// Created a Mqtt311 Connection from mqtt5Client. The options are setup by the builder.
std::shared_ptr<Aws::Crt::Mqtt::MqttConnection> mqttConnection =
Mqtt::MqttConnection::NewConnectionFromMqtt5Client(mqtt5Client);
ASSERT_TRUE(mqttConnection);

// Publish an offline message to create an incomplete publish operation
mqttConnection->Publish(testTopic.c_str(), Mqtt::QOS::AWS_MQTT_QOS_AT_LEAST_ONCE, false, testPayload, NULL);

delete builder;

// If the incomplete operation callback was not called, there would be a memory leak as the callback data was not
// released
Comment on lines +3207 to +3208
Copy link
Contributor

@sfod sfod Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Debatable: Maybe pass to Publish operation a callback which will set flag, something like is_callback_called. Then if callback is not called, we have a clear ASSERT statement indicating what the exact problem is. Currently, you have to go to this code and read this comment to figure out why memory leak appeared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about it, we actually could not find a good timing to validate the is_callback_called flag, because we do not know when does the callback is called during the termination process.

return AWS_OP_SUCCESS;
}
AWS_TEST_CASE(Mqtt5to3AdapterNullPubAck, s_TestMqtt5to3AdapterNullPubAck)

/*
* [Mqtt5to3Adapter-UC11] Test one mqtt5 client with multiple adapters
*/
Expand Down