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

[CLIENT-1337] Unmask C client errors #470

Merged
merged 8 commits into from
Sep 20, 2023
Merged

Conversation

juliannguyen4
Copy link
Collaborator

@juliannguyen4 juliannguyen4 commented Jul 18, 2023

Build wheels all passes: https://github.com/aerospike/aerospike-client-python/actions/runs/6240538285

Valgrind
Stage branch: https://github.com/aerospike/aerospike-client-python/actions/runs/6227886001
This branch: https://github.com/aerospike/aerospike-client-python/actions/runs/6240538715

jnguyen@as-PF3WN8Y6:~/aerospike/aerospike-client-python$ diff <(grep Aerospike stage.log | awk '{print $5}' | sort) <(grep Aerospike 'CLIENT-1337.log' | awk '{print $5}' | sort)
5a6,9
> AerospikeClient_Admin_Create_Role
> AerospikeClient_Admin_Create_Role
> AerospikeClient_Admin_Create_Role
> AerospikeClient_Admin_Create_User
13a18
> AerospikeClient_Admin_Drop_User
74a80
> AerospikeClient_New
105a112
> AerospikeClient_Remove
108a116,117
> AerospikeClient_Remove_Invoke
> AerospikeClient_Select
112a122
> AerospikeClient_Select_Invoke
124a135
> AerospikeClient_Type_Init
218d228
< AerospikeQuery_Get_Partitions_status

Extra leaks in this branch

All extra leaks in this branch come from raise_exception or raise_exception_old. This is because the errors from the C client are no longer being reset by the Python client, so I believe more as_error fields are being converted to Python objects and stored in the relevant exception class. Since the class isn't cleaned up properly, this causes more memory leaks from the class's attributes.

3 AerospikeClient_Admin_Create_Role leaks from raise_exception
1 AerospikeClient_Admin_Create_User leak from raise_exception

AerospikeClient_New:

Stage

5 from raise_exception
4 from AerospikeClient_Type_Init

This branch

5 from raise_exception
5 from AerospikeClient_Type_Init

1 AerospikeClient_Remove leak from raise_exception_old
1 AerospikeClient_Select leak from raise_exception_old

AerospikeClient_Type_Init:

Stage

5 from raise_exception
4 from as_config_add_host

This branch

5 from as_config_add_host
5 from raise_exception

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (39d0759) 81.30% compared to head (1a9338d) 81.31%.

❗ Current head 1a9338d differs from pull request most recent head ee96cc2. Consider uploading reports for the commit ee96cc2 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            stage     #470   +/-   ##
=======================================
  Coverage   81.30%   81.31%           
=======================================
  Files          98       98           
  Lines       14808    14757   -51     
=======================================
- Hits        12039    11999   -40     
+ Misses       2769     2758   -11     
Files Changed Coverage Δ
src/main/client/admin.c 81.29% <ø> (-0.12%) ⬇️
src/main/client/apply.c 86.02% <ø> (-0.15%) ⬇️
src/main/client/exists.c 92.85% <ø> (-0.11%) ⬇️
src/main/client/get.c 95.38% <ø> (-0.07%) ⬇️
src/main/client/operate.c 91.45% <ø> (-0.03%) ⬇️
src/main/client/put.c 97.18% <ø> (-0.08%) ⬇️
src/main/client/query.c 63.24% <ø> (+0.08%) ⬆️
src/main/client/remove.c 83.82% <ø> (-0.47%) ⬇️
src/main/client/remove_bin.c 84.40% <ø> (-0.42%) ⬇️
src/main/client/select.c 90.09% <ø> (-0.10%) ⬇️
... and 2 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@juliannguyen4 juliannguyen4 force-pushed the CLIENT-1337-unmask-c-errs branch from c150a44 to cc63f14 Compare August 4, 2023 17:27
@juliannguyen4 juliannguyen4 marked this pull request as ready for review August 4, 2023 17:31
@juliannguyen4 juliannguyen4 force-pushed the CLIENT-1337-unmask-c-errs branch from 941138e to cc63f14 Compare August 4, 2023 17:32
Copy link
Contributor

@dwelch-spike dwelch-spike left a comment

Choose a reason for hiding this comment

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

I haven't taken a very close look but my guess about the new memory leaks @juliannguyen4 mentioned is that they are coming from the new calls to raise_exception() that are hit because err.code isn't reset in many places now.

@juliannguyen4 juliannguyen4 force-pushed the CLIENT-1337-unmask-c-errs branch 3 times, most recently from 21657ae to fc8c900 Compare August 29, 2023 17:41
@juliannguyen4 juliannguyen4 force-pushed the CLIENT-1337-unmask-c-errs branch from fc8c900 to 977821d Compare September 19, 2023 19:38
@juliannguyen4 juliannguyen4 changed the title [CLIENT-1337] Unmask C client errors [CLIENT-1337] Unmask C client errors and fix strong consistency bug Sep 19, 2023
@juliannguyen4 juliannguyen4 force-pushed the CLIENT-1337-unmask-c-errs branch from 3689014 to 1a9338d Compare September 19, 2023 20:08
@juliannguyen4 juliannguyen4 changed the title [CLIENT-1337] Unmask C client errors and fix strong consistency bug [CLIENT-1337] Unmask C client errors Sep 19, 2023
Copy link
Contributor

@dwelch-spike dwelch-spike left a comment

Choose a reason for hiding this comment

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

LGTM

@juliannguyen4 juliannguyen4 merged commit 481cccf into stage Sep 20, 2023
@juliannguyen4 juliannguyen4 deleted the CLIENT-1337-unmask-c-errs branch September 20, 2023 00:28
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.

3 participants