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

kvs-watch: Fix matchtag leak on watcher destruction #1882

Merged
merged 3 commits into from Dec 20, 2018

Conversation

Projects
None yet
3 participants
@chu11
Copy link
Contributor

chu11 commented Dec 19, 2018

The main fix is the first commit, then there are two follow up minor cleanups.

@chu11 chu11 changed the title kvs-watch: Fix matchlog leak on watcher destruction kvs-watch: Fix matchtag leak on watcher destruction Dec 19, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 19, 2018

hmm, hit a valgrind error, which I also saw in #1874. Also another builder hung. Restarted them.

==32308== Memcheck, a memory error detector
==32308== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==32308== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==32308== Command: /usr/src/src/broker/.libs/flux-broker --shutdown-grace=4 /usr/src/t/valgrind/valgrind-workload.sh 10
==32308== 
hwloc x86 backend cannot work under Valgrind, disabling.
FLUX_URI=local:///tmp/flux-exUXe3
==32308== 
==32308== HEAP SUMMARY:
==32308==     in use at exit: 6,346,957 bytes in 182 blocks
==32308==   total heap usage: 955,373 allocs, 955,191 frees, 223,271,270 bytes allocated
==32308== 
==32308== 6,310,124 (16 direct, 6,310,108 indirect) bytes in 1 blocks are definitely lost in loss record 101 of 101
==32308==    at 0x4C2FA3F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32308==    by 0x4C31D84: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32308==    by 0x4E87EE9: ev_realloc (ev.c:1741)
==32308==    by 0x4E8CA58: ev_idle_start (ev.c:4611)
==32308==    by 0x4E61630: then_context_start (future.c:180)
==32308==    by 0x4E61630: post_fulfill.isra.6 (future.c:346)
==32308==    by 0x4E5E779: response_cb (rpc.c:170)
==32308==    by 0x4E597A6: call_handler (msg_handler.c:229)
==32308==    by 0x4E59EB8: dispatch_message (msg_handler.c:246)
==32308==    by 0x4E59EB8: handle_cb (msg_handler.c:322)
==32308==    by 0x4E87D12: ev_invoke_pending (ev.c:3314)
==32308==    by 0x4E8B2D8: ev_run (ev.c:3717)
==32308==    by 0x4E58952: flux_reactor_run (reactor.c:140)
==32308==    by 0xB7C910F: mod_main (job.c:938)
==32308==    by 0x1142EB: module_thread (module.c:158)
==32308==    by 0x55BA6DA: start_thread (pthread_create.c:463)
==32308==    by 0x636188E: clone (clone.S:95)
==32308== 
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   match-leak-kinds: definite
   fun:malloc
   fun:realloc
   fun:ev_realloc
   fun:ev_idle_start
   fun:then_context_start
   fun:post_fulfill.isra.6
   fun:response_cb
   fun:call_handler
   fun:dispatch_message
   fun:handle_cb
   fun:ev_invoke_pending
   fun:ev_run
   fun:flux_reactor_run
   fun:mod_main
   fun:module_thread
   fun:start_thread
   fun:clone
}
==32308== LEAK SUMMARY:
==32308==    definitely lost: 16 bytes in 1 blocks
==32308==    indirectly lost: 6,310,100 bytes in 96 blocks
==32308==      possibly lost: 0 bytes in 0 blocks
==32308==    still reachable: 36,321 bytes in 83 blocks
==32308==         suppressed: 520 bytes in 2 blocks
==32308== Reachable blocks (those to which a pointer was found) are not shown.
==32308== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==32308== 
==32308== For counts of detected and suppressed errors, rerun with: -v
==32308== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 1 from 1)
not ok 1 - valgrind reports no new errors on single broker run
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 19, 2018

Codecov Report

Merging #1882 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1882      +/-   ##
==========================================
+ Coverage   80.09%   80.12%   +0.02%     
==========================================
  Files         197      197              
  Lines       35015    35017       +2     
==========================================
+ Hits        28047    28058      +11     
+ Misses       6968     6959       -9
Impacted Files Coverage Δ
src/modules/kvs-watch/kvs-watch.c 78.88% <100%> (-0.13%) ⬇️
src/modules/kvs/kvs.c 66.49% <0%> (+0.14%) ⬆️
src/cmd/flux-module.c 83.96% <0%> (+0.23%) ⬆️
src/modules/connector-local/local.c 74.81% <0%> (+1.03%) ⬆️

@chu11 chu11 force-pushed the chu11:issue1880 branch from 26f40a3 to f852d34 Dec 20, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 20, 2018

rebased

chu11 added some commits Dec 18, 2018

modules/kvs-watch: Fix matchtag leak on lookups
If a watcher is destroyed before all RPCs have completed,
matchtags can leak.  Instead, when a watcher is done (e.g.
an error has been returned), set a flag indicating that
all further watch actions should not be done.  Wait for
remaining RPCs to silently complete before destroying
a watcher.

Fixes #1880
modules/kvs-watch: Refactor common cleanup code
Refactor common watcher cleanup and namespace cleanup code
into new watcher_cleanup() function.
modules/kvs-watch: Remove return value
Remove return value from handle_lookup_response(), as the return
value is not used.

@chu11 chu11 force-pushed the chu11:issue1880 branch from f852d34 to 9d564a6 Dec 20, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 20, 2018

rebased

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 20, 2018

LGTM thanks!

@garlick garlick merged commit 96d8949 into flux-framework:master Dec 20, 2018

3 checks passed

codecov/patch 100% of diff hit (target 80.09%)
Details
codecov/project 80.12% (+0.02%) compared to 5e4399c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.