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

modules/kvs/kvstxn: Fix mem-leak in kvs merge #1946

Merged
merged 3 commits into from Jan 23, 2019

Conversation

@chu11
Copy link
Contributor

commented Jan 22, 2019

In kvstxn_create_empty(), the 'keys' array was created that
could be overwritten without being destroyed. This was a corner
case in PR #1859, when key generation was altered.

Fixes #1944

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

In one run hit

FAIL: t0001-basic.t 55 - instance can stop cleanly with subscribers (#1025)
             t0001-basic.t:  FAIL: N=62  PASS=61  FAIL=1 SKIP=0 XPASS=0 XFAIL=0

which appears to be #1077.

But have never seen

FAIL: t2000-wreck.t 15 - wreckrun: --input different for each task works
             t2000-wreck.t:  FAIL: N=73  PASS=71  FAIL=1 SKIP=1 XPASS=0 XFAIL=0

debug shows

expecting success: 
        for i in `seq 0 3`; do echo "hi $i" > i.$i; done &&
        flux wreckrun --input="i.0:0;i.1:1;i.2:2;i.3:3" -l -n4 cat |
		sort -n > output.multi &&
        cat >expected.multi <<-EOF &&
	0: hi 0
	1: hi 1
	2: hi 2
	3: hi 3
	EOF
        test_cmp expected.multi output.multi

--- expected.multi	2019-01-22 21:31:31.304176846 +0000
+++ output.multi	2019-01-22 21:31:31.304176846 +0000
@@ -1,4 +1,3 @@
 0: hi 0
 1: hi 1
-2: hi 2
-3: hi 3
+2: hi 3
not ok 15 - wreckrun: --input different for each task works

which seems disturbing. But I can't imagine my stupid mem-leak fix here could be the cause of this. If my mem-leak fix were truly horrible, there'd be way more bugs than this one. @grondo, got any idea on this one?

@grondo

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

@grondo, got any idea on this one?

No and I'm not sure I've seen that failure before. Disturbingly, task 2 is getting the output from task 3 and task 3 is missing output. We should open a separate issue and I'll try to see if it can be reproduced.

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Actually, looks like the above wreckrun --input test failure was seen once before here...

@grondo

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

Well, that's embarrassing. I completely forgot about that.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

rebased

@codecov-io

This comment has been minimized.

Copy link

commented Jan 23, 2019

Codecov Report

Merging #1946 into master will increase coverage by <.01%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #1946      +/-   ##
==========================================
+ Coverage   80.03%   80.03%   +<.01%     
==========================================
  Files         195      195              
  Lines       34945    34931      -14     
==========================================
- Hits        27968    27957      -11     
+ Misses       6977     6974       -3
Impacted Files Coverage Δ
src/modules/kvs/kvstxn.c 77.15% <75%> (+0.32%) ⬆️
src/common/libflux/mrpc.c 87.74% <0%> (-1.19%) ⬇️
src/cmd/flux-module.c 83.72% <0%> (-0.48%) ⬇️
src/modules/kvs/kvs.c 66.35% <0%> (+0.14%) ⬆️
src/common/libutil/veb.c 98.85% <0%> (+0.57%) ⬆️
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

actually hold off on a merge, I realized I could refactor this a tiny bit to prevent unexpected errors from coming up in the future. I can't remember why I decided to make separate kvstxn_create() and kvstxn_create_empty() functions in the past, but it's not necessary.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

re-pushed with a few refactoring fixes

@chu11 chu11 force-pushed the chu11:issue1944 branch 2 times, most recently from 56a84da to e8bf6ca Jan 23, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

rebased

chu11 added 3 commits Jan 22, 2019
In kvstxn_create_empty(), the 'keys' array was created that
could be overwritten without being destroyed.  This was a corner
case in PR #1859, when key generation was altered.

Fixes #1944
Allow kvstxn_create() to take several NULL pointers, so that
kvstxn_create_empty() can be removed.
All "goto error" paths were ENOMEM errnos, so cleanup code
by not setting "errno = ENOMEM" in all paths.
@chu11 chu11 force-pushed the chu11:issue1944 branch from e8bf6ca to ff7a4a2 Jan 23, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

rebased

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

Thanks!

@garlick garlick merged commit 34ed5f6 into flux-framework:master Jan 23, 2019
2 of 3 checks passed
2 of 3 checks passed
codecov/patch 75% of diff hit (target 80.03%)
Details
codecov/project 80.03% (+<.01%) compared to d3ee8a1
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
Projects
None yet
4 participants
You can’t perform that action at this time.