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.

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

Add initial skeleton for HealthClient API in mssf_core #114

Merged
merged 12 commits into from
Jan 8, 2025

Conversation

OscarTHZhang
Copy link
Contributor

@OscarTHZhang OscarTHZhang commented Dec 27, 2024

Add initial skeleton for HealthClient API in mssf_core with health report types and report health. Other health client functions are left as todos.

@cgettys-microsoft
Copy link
Contributor

Seems reasonable to me, for what that's worth :).

Self {
PartitionId: value.partition_id,
ReplicaId: value.replica_id,
HealthInformation: Box::into_raw(boxed_health_info),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Who is responsible to destroy/free the raw ptr? This is a mem leak.

Copy link
Contributor Author

@OscarTHZhang OscarTHZhang Jan 8, 2025

Choose a reason for hiding this comment

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

See the latest change - I created a utility wrapper on top of the raw COM types, which calls Box::from_raw() in drop. There should be no mem leak according to valgrind.

@OscarTHZhang
Copy link
Contributor Author

Here is the valgrind output on the conversion tests on latest commit. No memory leaks detected:

test types::client::health::test::test_cluster_health_report_wrapper_conversion ... ok
test types::client::health::test::test_application_health_report_conversion ... ok
test types::client::health::test::test_stateful_service_replica_health_report_conversion ... ok
test types::client::health::test::test_stateful_service_replica_health_report_wrapper_conversion ... ok
test types::client::health::test::test_cluster_health_report_conversion ... ok
test types::client::health::test::test_node_health_report_wrapper_conversion ... ok
test types::client::health::test::test_partition_health_report_conversion ... ok
test types::client::health::test::test_partition_health_report_wrapper_conversion ... ok
test types::client::health::test::test_service_health_report_wrapper_conversion ... ok
test types::client::health::test::test_invalid_health_report_conversion ... ok
test types::client::health::test::test_application_health_report_wrapper_conversion ... ok
test types::client::health::test::test_deployed_service_package_health_report_conversion ... ok
test types::client::health::test::test_deployed_service_package_health_report_wrapper_conversion ... ok
test types::client::health::test::test_deployed_application_health_report_wrapper_conversion ... ok
test types::client::health::test::test_stateless_service_instance_health_report_conversion ... ok
test types::client::health::test::test_service_health_report_conversion ... ok
test types::client::health::test::test_stateless_service_instance_health_report_wrapper_conversion ... ok
test types::client::health::test::test_deployed_application_health_report_conversion ... ok

test result: ok. 19 passed; 0 failed; 0 ignored; 0 measured; 20 filtered out; finished in 0.33s

==3055234==
==3055234== HEAP SUMMARY:
==3055234==     in use at exit: 0 bytes in 0 blocks
==3055234==   total heap usage: 1,332 allocs, 1,332 frees, 147,258 bytes allocated
==3055234==
==3055234== All heap blocks were freed -- no leaks are possible

@youyuanwu
Copy link
Collaborator

Seems like u got access violation in the windows tests. The box into_raw should be unsafe code and needs special care. Also I don't know what Rust optimizer would do.

@OscarTHZhang
Copy link
Contributor Author

The type conversion using From is deemed unsafe - release test failed with segfault. The latest change uses stack instead

crates/libs/core/src/types/client/health.rs Outdated Show resolved Hide resolved
@OscarTHZhang OscarTHZhang merged commit 98d56e4 into Azure:main Jan 8, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants