-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(dgraph): Panic because of nil map in groups.go #5999
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @manishrjain, @parasssh, and @vvbalaji-dgraph)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, just remembered why this is done the way it is now. The existing delta might have checksums already. The fix should be to create the map if it is nil.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @manishrjain, @parasssh, and @vvbalaji-dgraph)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Martin. Lets make
the map when the "delta" variable is declared on line 926.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @manishrjain, @parasssh, and @vvbalaji-dgraph)
f2a1267
to
bf955fa
Compare
delta.GroupChecksums is nil at this point in the code, so copying keys to this map causes the code to panic
bf955fa
to
4f91551
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain, @martinmr, @rahulgurnani, and @vvbalaji-dgraph)
worker/groups.go, line 926 at r2 (raw file):
for { var delta *pb.OracleDelta
can we do the make here itself? That way we dont have to do an if check in the for loop below.
@martinmr Addressed your review comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont see it addressed.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain, @martinmr, @rahulgurnani, and @vvbalaji-dgraph)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain, @martinmr, @parasssh, and @vvbalaji-dgraph)
worker/groups.go, line 926 at r2 (raw file):
I think if we keep it nearer to where it's used, code will be easier to understand. Also we may be allocating it would have to garbage collected if delta gets the GroupChecksums map from somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I amended my previous commit, please check the git diff
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain, @martinmr, @parasssh, and @vvbalaji-dgraph)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain, @martinmr, @rahulgurnani, and @vvbalaji-dgraph)
worker/groups.go, line 926 at r2 (raw file):
Previously, rahulgurnani (Rahul Gurnani) wrote…
I think if we keep it nearer to where it's used, code will be easier to understand. Also we may be allocating it would have to garbage collected if delta gets the GroupChecksums map from somewhere else.
I dont think that should be a concern. Currently we dont just assign it and make copy. I think make
here is cleaner. Also, it is slightly better than doing the if check in the for loop on every iteration.
Remove if nil check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain, @martinmr, @parasssh, and @vvbalaji-dgraph)
worker/groups.go, line 926 at r2 (raw file):
Previously, parasssh wrote…
I dont think that should be a concern. Currently we dont just assign it and make copy. I think
make
here is cleaner. Also, it is slightly better than doing the if check in the for loop on every iteration.
Okay, Updated the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain, @martinmr, @parasssh, and @vvbalaji-dgraph)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain, @parasssh, and @vvbalaji-dgraph)
Can you run your tests locally. Lots of tests are failing so maybe the change is not as benign as we thought. But possible that this is just TeamCity acting up. |
All CI tests pass locally. |
Actually, the code was faulty. Made the fix to do a make on the map just before using it in the for loop as Rahul had it earlier. |
* Initialize delta.GroupChecksums if nil delta.GroupChecksums is nil at this point in the code, so copying keys to this map causes the code to panic * Address review comments Remove if nil check * fix CI; move map make to for loop since delta is assigned a struct from the deltaCh (cherry picked from commit 3c46878)
* Initialize delta.GroupChecksums if nil delta.GroupChecksums is nil at this point in the code, so copying keys to this map causes the code to panic * Address review comments Remove if nil check * fix CI; move map make to for loop since delta is assigned a struct from the deltaCh (cherry picked from commit 3c46878)
* Initialize delta.GroupChecksums if nil delta.GroupChecksums is nil at this point in the code, so copying keys to this map causes the code to panic * Address review comments Remove if nil check * fix CI; move map make to for loop since delta is assigned a struct from the deltaCh (cherry picked from commit 3c46878)
* Initialize delta.GroupChecksums if nil delta.GroupChecksums is nil at this point in the code, so copying keys to this map causes the code to panic * Address review comments Remove if nil check * fix CI; move map make to for loop since delta is assigned a struct from the deltaCh
Fixes DGRAPH-1935
delta.GroupChecksums is nil at this point in the code, so copying keys to this map causes the code to panic with message:
assignment to entry in nil map
This change is