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

Problem: possible memory leak in deregistering the client #261

Merged
merged 1 commit into from
Jun 28, 2017

Conversation

lerwys
Copy link
Contributor

@lerwys lerwys commented Jun 26, 2017

When deregistering the client, it deletes the client
hash entry and then the mlm_server_engine.inc FSM
calls allow_time_to_settle ().

This routine currently sets a timer of 1000ms
and then teminate the client by calling
the zhash destructor and then client_terminate ().

This should be sufficient to free client internal
structures as client_terminate () frees the client
address, but for some reason valgrind shows memory
leaks, as shown below:

==5909== Memcheck, a memory error detector
==5909== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==5909== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright
info
==5909== Command: /usr/bin/malamute -f /usr/etc/malamute/malamute.cfg
==5909== Parent PID: 1
==5909==
==5909==
==5909== HEAP SUMMARY:
==5909== in use at exit: 27,966 bytes in 1,441 blocks
==5909== total heap usage: 3,721,994 allocs, 3,720,553 frees,
489,052,905 bytes allocated
==5909==
==5909== 369 bytes in 19 blocks are definitely lost in loss record 1 of
2
==5909== at 0x4C27BE3: malloc (vg_replace_malloc.c:299)
==5909== by 0x5636889: strdup (in /usr/lib64/libc-2.17.so)
==5909== by 0x4E40B99: register_new_client (mlm_server.c:403)
==5909== by 0x4E3ECF4: s_client_execute (mlm_server_engine.inc:504)
==5909== by 0x4E40FD3: s_server_handle_protocol
(mlm_server_engine.inc:1551)
==5909== by 0x532F87C: zloop_start (zloop.c:794)
==5909== by 0x4E40E00: mlm_server (mlm_server_engine.inc:1598)
==5909== by 0x5317EF2: s_thread_shim (zactor.c:67)
==5909== by 0x5B80DC4: start_thread (in /usr/lib64/libpthread-2.17.so)
==5909== by 0x56A776C: clone (in /usr/lib64/libc-2.17.so)
==5909==
==5909== 27,597 bytes in 1,422 blocks are definitely lost in loss record
2 of 2
==5909== at 0x4C27BE3: malloc (vg_replace_malloc.c:299)
==5909== by 0x5636889: strdup (in /usr/lib64/libc-2.17.so)
==5909== by 0x4E40B99: register_new_client (mlm_server.c:403)
==5909== by 0x4E3F024: s_client_execute (mlm_server_engine.inc:1221)
==5909== by 0x4E40FD3: s_server_handle_protocol
(mlm_server_engine.inc:1551)
==5909== by 0x532F87C: zloop_start (zloop.c:794)
==5909== by 0x4E40E00: mlm_server (mlm_server_engine.inc:1598)
==5909== by 0x5317EF2: s_thread_shim (zactor.c:67)
==5909== by 0x5B80DC4: start_thread (in /usr/lib64/libpthread-2.17.so)
==5909== by 0x56A776C: clone (in /usr/lib64/libc-2.17.so)
==5909==
==5909== LEAK SUMMARY:
==5909== definitely lost: 27,966 bytes in 1,441 blocks
==5909== indirectly lost: 0 bytes in 0 blocks
==5909== possibly lost: 0 bytes in 0 blocks
==5909== still reachable: 0 bytes in 0 blocks
==5909== suppressed: 0 bytes in 0 blocks
==5909==
==5909== For counts of detected and suppressed errors, rerun with: -v
==5909== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

Solution: free client address and set it to NULL
to avoid segfaults when timer expires and it tries
to free it later on

This is not completely understood, but this seems
to avoid memory leaks in valgrind.

When deregistering the client, it deletes the client
hash entry and then the mlm_server_engine.inc FSM
calls allow_time_to_settle ().

This routine currently sets a timer of 1000ms
and then teminate the client by calling
the zhash destructor and then client_terminate ().

This should be sufficient to free client internal
structures as client_terminate () frees the client
address, but for some reason valgrind shows memory
leaks, as shown below:

==5909== Memcheck, a memory error detector
==5909== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==5909== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright
info
==5909== Command: /usr/bin/malamute -f /usr/etc/malamute/malamute.cfg
==5909== Parent PID: 1
==5909==
==5909==
==5909== HEAP SUMMARY:
==5909==     in use at exit: 27,966 bytes in 1,441 blocks
==5909==   total heap usage: 3,721,994 allocs, 3,720,553 frees,
489,052,905 bytes allocated
==5909==
==5909== 369 bytes in 19 blocks are definitely lost in loss record 1 of
2
==5909==    at 0x4C27BE3: malloc (vg_replace_malloc.c:299)
==5909==    by 0x5636889: strdup (in /usr/lib64/libc-2.17.so)
==5909==    by 0x4E40B99: register_new_client (mlm_server.c:403)
==5909==    by 0x4E3ECF4: s_client_execute (mlm_server_engine.inc:504)
==5909==    by 0x4E40FD3: s_server_handle_protocol
(mlm_server_engine.inc:1551)
==5909==    by 0x532F87C: zloop_start (zloop.c:794)
==5909==    by 0x4E40E00: mlm_server (mlm_server_engine.inc:1598)
==5909==    by 0x5317EF2: s_thread_shim (zactor.c:67)
==5909==    by 0x5B80DC4: start_thread (in /usr/lib64/libpthread-2.17.so)
==5909==    by 0x56A776C: clone (in /usr/lib64/libc-2.17.so)
==5909==
==5909== 27,597 bytes in 1,422 blocks are definitely lost in loss record
2 of 2
==5909==    at 0x4C27BE3: malloc (vg_replace_malloc.c:299)
==5909==    by 0x5636889: strdup (in /usr/lib64/libc-2.17.so)
==5909==    by 0x4E40B99: register_new_client (mlm_server.c:403)
==5909==    by 0x4E3F024: s_client_execute (mlm_server_engine.inc:1221)
==5909==    by 0x4E40FD3: s_server_handle_protocol
(mlm_server_engine.inc:1551)
==5909==    by 0x532F87C: zloop_start (zloop.c:794)
==5909==    by 0x4E40E00: mlm_server (mlm_server_engine.inc:1598)
==5909==    by 0x5317EF2: s_thread_shim (zactor.c:67)
==5909==    by 0x5B80DC4: start_thread (in /usr/lib64/libpthread-2.17.so)
==5909==    by 0x56A776C: clone (in /usr/lib64/libc-2.17.so)
==5909==
==5909== LEAK SUMMARY:
==5909==    definitely lost: 27,966 bytes in 1,441 blocks
==5909==    indirectly lost: 0 bytes in 0 blocks
==5909==      possibly lost: 0 bytes in 0 blocks
==5909==    still reachable: 0 bytes in 0 blocks
==5909==         suppressed: 0 bytes in 0 blocks
==5909==
==5909== For counts of detected and suppressed errors, rerun with: -v
==5909== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

Solution: free client address and set it to NULL
to avoid segfaults when timer expires and it tries
to free it later on

This is not completely understood, but this seems
to avoid memory leaks in valgrind.
@vyskocilm vyskocilm merged commit cd4d0ff into zeromq:master Jun 28, 2017
zhashx_delete (self->server->clients, self->address);
zstr_free (&self->address);
Copy link
Member

Choose a reason for hiding this comment

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

The cleaner solution would be to set a key_destructor

void zhashx_set_key_destructor (zhashx_t *self, zhashx_destructor_fn destructor)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure. I'll make another PR for this. But it's just weird to have a destructor that doesn't actually destroy the instance, just some of its internal properties.

client_terminate () should be the routine responsible for this I think, right?

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