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 & kvs-watch: misc cleanup & bug fixes #1861

Merged
merged 7 commits into from Dec 8, 2018

Conversation

Projects
None yet
3 participants
@chu11
Copy link
Contributor

chu11 commented Dec 7, 2018

while working on #1838, misc cleanups and bug fixes.

chu11 added some commits Dec 3, 2018

modules/kvs: Add assert on fallthrough path
Add an assert to handle a fallthrough path that should be
impossible.
modules/kvs: Fix potential invalid errno
Fix corner case in which call to helper function could overwrite
the desired errno to return.
modules/kvs: Refactor lookup fallthrough case
Remove unnecessary 'else' clause, just fallthrough to rest of
code.
modules/kvs-watch: Fix ENOTSUP corner case
Fix a corner case when a namespace being monitored is removed, by
assigning the ENOTSUP errnum to a "fatal error" variable.  The ENOTSUP
from a removed namespace event is not subject to FLUX_KVS_WATCH_WAITCREATE,
this one truly indicates the namespace is gone.
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 7, 2018

one build had a valgrind error. Maybe #1641? but seems to run through job.c.

expecting success: 
	run_timeout 120 \
	libtool e flux ${VALGRIND} \
		--tool=memcheck \
		--leak-check=full \
		--gen-suppressions=all \
		--trace-children=no \
		--child-silent-after-fork=yes \
		--num-callers=30 \
		--leak-resolution=med \
		--error-exitcode=1 \
		--suppressions=$VALGRIND_SUPPRESSIONS \
		${BROKER} --shutdown-grace=4 ${VALGRIND_WORKLOAD} 10
==805== Memcheck, a memory error detector
==805== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==805== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==805== Command: /usr/src/src/broker/.libs/flux-broker --shutdown-grace=4 /usr/src/t/valgrind/valgrind-workload.sh 10
==805== 
hwloc x86 backend cannot work under Valgrind, disabling.
FLUX_URI=local:///tmp/flux-bpPEvV
==805== 
==805== HEAP SUMMARY:
==805==     in use at exit: 6,347,963 bytes in 182 blocks
==805==   total heap usage: 958,977 allocs, 958,795 frees, 223,721,477 bytes allocated
==805== 
==805== 6,311,130 (16 direct, 6,311,114 indirect) bytes in 1 blocks are definitely lost in loss record 98 of 98
==805==    at 0x4C2FA3F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==805==    by 0x4C31D84: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==805==    by 0x4E88299: ev_realloc (ev.c:1741)
==805==    by 0x4E8CE08: ev_idle_start (ev.c:4611)
==805==    by 0x4E61360: then_context_start (future.c:180)
==805==    by 0x4E61360: post_fulfill.isra.6 (future.c:346)
==805==    by 0x4E5E439: response_cb (rpc.c:170)
==805==    by 0x4E59466: call_handler (msg_handler.c:229)
==805==    by 0x4E59B78: dispatch_message (msg_handler.c:246)
==805==    by 0x4E59B78: handle_cb (msg_handler.c:322)
==805==    by 0x4E880C2: ev_invoke_pending (ev.c:3314)
==805==    by 0x4E8B688: ev_run (ev.c:3717)
==805==    by 0x4E58612: flux_reactor_run (reactor.c:140)
==805==    by 0xB7CD10F: mod_main (job.c:938)
==805==    by 0x11433B: module_thread (module.c:158)
==805==    by 0x55BB6DA: start_thread (pthread_create.c:463)
==805==    by 0x636288E: clone (clone.S:95)
==805== 
{
   <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
}
==805== LEAK SUMMARY:
==805==    definitely lost: 16 bytes in 1 blocks
==805==    indirectly lost: 6,311,106 bytes in 96 blocks
==805==      possibly lost: 0 bytes in 0 blocks
==805==    still reachable: 36,321 bytes in 83 blocks
==805==         suppressed: 520 bytes in 2 blocks
==805== Reachable blocks (those to which a pointer was found) are not shown.
==805== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==805== 
==805== For counts of detected and suppressed errors, rerun with: -v
==805== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 1 from 1)
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 7, 2018

just saw there were some ';;' typos in some code, so just added that patch on top.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 7, 2018

Codecov Report

Merging #1861 into master will increase coverage by <.01%.
The diff coverage is 69.56%.

@@            Coverage Diff             @@
##           master    #1861      +/-   ##
==========================================
+ Coverage   79.89%   79.89%   +<.01%     
==========================================
  Files         196      196              
  Lines       35222    35228       +6     
==========================================
+ Hits        28141    28147       +6     
  Misses       7081     7081
Impacted Files Coverage Δ
src/modules/kvs-watch/kvs-watch.c 76.49% <44.44%> (+0.34%) ⬆️
src/modules/kvs/kvs.c 65.61% <85.71%> (-0.05%) ⬇️
src/common/libflux/message.c 81.39% <0%> (-0.13%) ⬇️
src/cmd/flux-module.c 84.93% <0%> (+0.24%) ⬆️
src/broker/modservice.c 79.8% <0%> (+0.96%) ⬆️

@chu11 chu11 force-pushed the chu11:issue1838-cleanup branch from b43c6a1 to edc1082 Dec 7, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 7, 2018

oh wait, i just realized one of the bug fixes isn't necessary ... ugh. let me remove it.

Edit: What I realized was that watcher_destroy() destroys all futures that are connected to the watcher, so it's not possible for the lookup continuation to be called.

@chu11 chu11 force-pushed the chu11:issue1838-cleanup branch from edc1082 to 306e661 Dec 7, 2018

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 8, 2018

LGTM! Thanks!

@garlick garlick merged commit 52068c6 into flux-framework:master Dec 8, 2018

2 of 3 checks passed

codecov/patch 69.56% of diff hit (target 79.89%)
Details
codecov/project 79.89% (+<.01%) compared to 81b5fe0
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.