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

[Example] Multi-threaded programming example #39

Merged
merged 1 commit into from
Nov 30, 2020

Conversation

ityuhui
Copy link
Member

@ityuhui ityuhui commented Nov 12, 2020

Alougth I use pthread_cleanup_push()/pthread_cleanup_pop() after pthread_cancel() for the watch thread. But some resources still leak because they are allocated in the functions from openapi-generator/c-libcurl, their handles(pointers) are hard to get.

So it seems that the endless watch thread is better, should I change the example to remove pthread_cleanup_push()/pthread_cleanup_pop()/pthread_cancel(), run the watch thread for ever ?

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 12, 2020
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 12, 2020
@ityuhui
Copy link
Member Author

ityuhui commented Nov 12, 2020

Append the result of valgrind:

valgrind --tool=memcheck --leak-check=full ./multi_thread_bin

==32218==
==32218== HEAP SUMMARY:
==32218== in use at exit: 134,418 bytes in 411 blocks
==32218== total heap usage: 12,293 allocs, 11,882 frees, 1,776,892 bytes allocated
==32218==
==32218== 23 bytes in 1 blocks are definitely lost in loss record 39 of 241
==32218== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32218== by 0x5037E0C: CoreV1API_listNamespacedPod (CoreV1API.c:19391)
==32218== by 0x10986C: watch_pod_thread_func (watch_pod.c:84)
==32218== by 0x557B6DA: start_thread (pthread_create.c:463)
==32218== by 0x58B4A3E: clone (clone.S:95)
==32218==
==32218== 32 bytes in 1 blocks are definitely lost in loss record 94 of 241
==32218== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32218== by 0x4EA1593: strReplace (apiClient.c:491)
==32218== by 0x5037E49: CoreV1API_listNamespacedPod (CoreV1API.c:19394)
==32218== by 0x10986C: watch_pod_thread_func (watch_pod.c:84)
==32218== by 0x557B6DA: start_thread (pthread_create.c:463)
==32218== by 0x58B4A3E: clone (clone.S:95)
==32218==
==32218== 68 bytes in 1 blocks are definitely lost in loss record 132 of 241
==32218== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32218== by 0x4EA0407: assembleTargetUrl (apiClient.c:140)
==32218== by 0x4EA1127: apiClient_invoke (apiClient.c:402)
==32218== by 0x50383F6: CoreV1API_listNamespacedPod (CoreV1API.c:19514)
==32218== by 0x10986C: watch_pod_thread_func (watch_pod.c:84)
==32218== by 0x557B6DA: start_thread (pthread_create.c:463)
==32218== by 0x58B4A3E: clone (clone.S:95)
==32218==
==32218== 86 (24 direct, 62 indirect) bytes in 1 blocks are definitely lost in loss record 146 of 241
==32218== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32218== by 0x4E9F917: list_create (list.c:27)
==32218== by 0x5037D72: CoreV1API_listNamespacedPod (CoreV1API.c:19373)
==32218== by 0x10986C: watch_pod_thread_func (watch_pod.c:84)
==32218== by 0x557B6DA: start_thread (pthread_create.c:463)
==32218== by 0x58B4A3E: clone (clone.S:95)
==32218==
==32218== 144 (24 direct, 120 indirect) bytes in 1 blocks are definitely lost in loss record 167 of 241
==32218== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32218== by 0x4E9F917: list_create (list.c:27)
==32218== by 0x5037D90: CoreV1API_listNamespacedPod (CoreV1API.c:19376)
==32218== by 0x10986C: watch_pod_thread_func (watch_pod.c:84)
==32218== by 0x557B6DA: start_thread (pthread_create.c:463)
==32218== by 0x58B4A3E: clone (clone.S:95)
==32218==
==32218== 134,065 (21,224 direct, 112,841 indirect) bytes in 1 blocks are definitely lost in loss record 241 of 241
==32218== at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32218== by 0x5BA309E: ??? (in /usr/lib/x86_64-linux-gnu/libcurl.so.4.5.0)
==32218== by 0x5BB0103: curl_easy_init (in /usr/lib/x86_64-linux-gnu/libcurl.so.4.5.0)
==32218== by 0x4EA0745: apiClient_invoke (apiClient.c:206)
==32218== by 0x50383F6: CoreV1API_listNamespacedPod (CoreV1API.c:19514)
==32218== by 0x10986C: watch_pod_thread_func (watch_pod.c:84)
==32218== by 0x557B6DA: start_thread (pthread_create.c:463)
==32218== by 0x58B4A3E: clone (clone.S:95)
==32218==
==32218== LEAK SUMMARY:
==32218== definitely lost: 21,395 bytes in 6 blocks
==32218== indirectly lost: 113,023 bytes in 405 blocks
==32218== possibly lost: 0 bytes in 0 blocks
==32218== still reachable: 0 bytes in 0 blocks
==32218== suppressed: 0 bytes in 0 blocks
==32218==
==32218== For counts of detected and suppressed errors, rerun with: -v
==32218== ERROR SUMMARY: 6 errors from 6 contexts (suppressed: 0 from 0)

@ityuhui
Copy link
Member Author

ityuhui commented Nov 12, 2020

/cc @brendandburns

@brendandburns
Copy link
Contributor

Perhaps we can use CURLOPT_PROGRESSFUNCTION to abort more cleanly?

https://curl.se/libcurl/c/CURLOPT_PROGRESSFUNCTION.html

I really think there has to be a better approach than an endless watch thread.

@ityuhui
Copy link
Member Author

ityuhui commented Nov 14, 2020

OK. I will go to investigate CURLOPT_PROGRESSFUNCTION

@brendandburns
Copy link
Contributor

Actually looks like there is a newer callback function: https://curl.se/libcurl/c/CURLOPT_XFERINFOFUNCTION.html

@ityuhui
Copy link
Member Author

ityuhui commented Nov 17, 2020

Hi @brendandburns

Thank you for your comments, the new code (using CURLOPT_PROGRESSFUNCTION ) can exit watch thread gracefully now. Could you please review the new code change ?

The change of apiClient.{h,c} will not be committed by this PR. So I label WIP to block the automatic code merge.


Alought there is still one memory leak, it's a previous defect existing in openapi-generator/c-libcurl. I will fix it in another PR.
==19621== 16 bytes in 1 blocks are definitely lost in loss record 1 of 1
==19621== at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19621== by 0x503A547: CoreV1API_listNamespacedPod (CoreV1API.c:19504)
==19621== by 0x109858: watch_pod_thread_func (watch_pod.c:91)
==19621== by 0x55946DA: start_thread (pthread_create.c:463)
==19621== by 0x58CDA3E: clone (clone.S:95)

#include <apiClient.h>
#include <CoreV1API.h>

typedef struct kube_params_t {
Copy link
Contributor

Choose a reason for hiding this comment

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

fix indentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@brendandburns
Copy link
Contributor

This looks good to me. Thanks for finding and fixing the api client too!

@ityuhui
Copy link
Member Author

ityuhui commented Nov 29, 2020

Hi @brendandburns

The libcurl progress function support is merged by #40
and the review comment of this PR is addressed too.

Could you please approve this PR ?


I will create another PR to fix the pre memory-leak issue of openapi-generator/c-libcurl

==19621== by 0x503A547: CoreV1API_listNamespacedPod (CoreV1API.c:19504)

@ityuhui ityuhui changed the title [WIP][Example] Multi-threaded programming example [Example] Multi-threaded programming example Nov 29, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2020
@brendandburns
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, ityuhui

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [brendandburns,ityuhui]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit e7015c8 into kubernetes-client:master Nov 30, 2020
@ityuhui ityuhui deleted the yh-mt-demo branch December 1, 2020 02:13
@ityuhui ityuhui added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 11, 2020
@ityuhui
Copy link
Member Author

ityuhui commented Dec 14, 2020

All of memory-leak issues are fixed by #42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants