Skip to content

Conversation

@ktf
Copy link
Member

@ktf ktf commented Apr 22, 2024

CCDBApi: avoid leaking curl handle.

@ktf ktf requested review from a team, Barthelemy, costing and sawenzel as code owners April 22, 2024 10:37
@github-actions
Copy link
Contributor

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass3
async-2023-pbpb-apass4
async-2022-pp-apass6-2023-PbPb-apass2
async-2022-pp-apass4
async-2022-pp-apass4-accepted
async-2022-pp-apass6-2023-PbPb-apass2-accepted
async-2023-pbpb-apass3-accepted
async-2023-pbpb-apass4-accepted

costing
costing previously approved these changes Apr 22, 2024
@martenole
Copy link
Contributor

Hi @ktf
thanks, this looks very good and if I run the FST with it the leak rate is strongly reduced. It seems the build checks are failing though, could you check please?

/sw/SOURCES/O2/13061-slc7_x86-64/0/CCDB/test/testCcdbApiDownloader.cxx(173): �[1;31;49merror: in "asynch_schedule_test": check httpCode == 200 has failed�[0;39;49m

@ktf
Copy link
Member Author

ktf commented Apr 22, 2024

I frankly think the test is wrong, but indeed it needs understanding. @costing could you cc Michal? I do not recall his handle.

@ktf
Copy link
Member Author

ktf commented Apr 22, 2024

Also the rest of the leak comes from the HTTPHEADER slist, however the way the code is currenlty structured, it's not easy to clean it up correctly.

@costing
Copy link
Collaborator

costing commented Apr 22, 2024

Adding @TrifleMichael

@ktf
Copy link
Member Author

ktf commented Apr 22, 2024

Updated with the fix for the remaining leak. I think the delete in the test should not be there, but someone should probably doublecheck the logic. With this, my reproducer does not show any leak whatsoever.

Copy link
Contributor

@martenole martenole left a comment

Choose a reason for hiding this comment

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

Looks good. I don't understand why the formatting check fails

@martenole martenole merged commit 35d6be7 into AliceO2Group:dev Apr 23, 2024
@ktf
Copy link
Member Author

ktf commented Apr 23, 2024

I think it's some glitch on the github side, actually. The code formats fine locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants