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

Pad encData to 17 bytes before decoding #4066

Merged
merged 2 commits into from
Sep 27, 2019
Merged

Conversation

animesh2049
Copy link
Contributor

@animesh2049 animesh2049 commented Sep 26, 2019

Fixes #4020
We tried to reproduce the issue.

Setup

  • 1 Alpha
  • 1 Zero
  • Live loader processing 1 million dataset

Linux

  • We couldn't reproduce the issue. It works fine irrespective of length of the encoded data given that we append at least 3 bytes.

Windows

  • Doesn't work with padding of only 3 bytes. Stack trace is below.
  • Works fine when concurrency parameter is set to 1 in live loader
  • Works fine when encoded data is appended with enough 0 bytes to ensure a length of 17 bytes
(dlv) bt                                                                                                                                                                                                                  0  0x0000000000437ce0 in runtime.fatalthrow
    at c:/go/src/runtime/panic.go:820
 1  0x0000000000437b69 in runtime.throw
    at c:/go/src/runtime/panic.go:774
 2  0x000000000044c443 in runtime.sigpanic
    at c:/go/src/runtime/signal_windows.go:236
 3  0x0000000000fe79b1 in github.com/dgraph-io/dgraph/vendor/github.com/dgryski/go-groupvarint.Decode4
    at C:/Users/manga/go/src/github.com/dgraph-io/dgraph/vendor/github.com/dgryski/go-groupvarint/decode_amd64.s:10
 4  0x0000000000fe9baa in github.com/dgraph-io/dgraph/codec.(*Decoder).unpackBlock.func1
    at C:/Users/manga/go/src/github.com/dgraph-io/dgraph/codec/codec.go:159
 5  0x0000000000fe86aa in github.com/dgraph-io/dgraph/codec.(*Decoder).unpackBlock
    at C:/Users/manga/go/src/github.com/dgraph-io/dgraph/codec/codec.go:160
 6  0x0000000000fe8b02 in github.com/dgraph-io/dgraph/codec.(*Decoder).Seek
    at C:/Users/manga/go/src/github.com/dgraph-io/dgraph/codec/codec.go:192
 7  0x0000000001004b46 in github.com/dgraph-io/dgraph/posting.(*pIterator).init
    at C:/Users/manga/go/src/github.com/dgraph-io/dgraph/posting/list.go:127
 8  0x0000000001007fa2 in github.com/dgraph-io/dgraph/posting.(*List).iterate
    at C:/Users/manga/go/src/github.com/dgraph-io/dgraph/posting/list.go:615
 9  0x0000000001008800 in github.com/dgraph-io/dgraph/posting.(*List).length
    at C:/Users/manga/go/src/github.com/dgraph-io/dgraph/posting/list.go:691
10  0x0000000000ffe520 in github.com/dgraph-io/dgraph/posting.(*Txn).addMutationHelper
    at C:/Users/manga/go/src/github.com/dgraph-io/dgraph/posting/index.go:356
11  0x0000000000ffefdd in github.com/dgraph-io/dgraph/posting.(*List).AddMutationWithIndex
    at C:/Users/manga/go/src/github.com/dgraph-io/dgraph/posting/index.go:394
12  0x00000000011d4b8b in github.com/dgraph-io/dgraph/worker.runMutation
    at C:/Users/manga/go/src/github.com/dgraph-io/dgraph/worker/mutation.go:115
13  0x000000000120b9f4 in github.com/dgraph-io/dgraph/worker.(*node).applyMutations.func3
    at C:/Users/manga/go/src/github.com/dgraph-io/dgraph/worker/draft.go:289
14  0x000000000120bc49 in github.com/dgraph-io/dgraph/worker.(*node).applyMutations.func4
    at C:/Users/manga/go/src/github.com/dgraph-io/dgraph/worker/draft.go:318
15  0x0000000000468131 in runtime.goexit
    at c:/go/src/runtime/asm_amd64.s:1357

This change is Reviewable

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ A review job has been created and sent to the PullRequest network.


@animesh2049 you can click here to see the review status or cancel the code review job.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'd recommend moving 17 into a variable with a descriptive name or comment, especially since it's now used in two places. But no biggie either way


Reviewed with ❤️ by PullRequest

@dgryski
Copy link

dgryski commented Sep 26, 2019

Might want to note this is to workaround dgryski/go-groupvarint#1

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm: except for a small question.

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @animesh2049 and @manishrjain)


codec/codec.go, line 142 at r1 (raw file):

			// decoding and expects it to be of length >= 4 at all the stages. Padding
			// with zero to make sure length is always >= 4.
			encData = append(encData, bytes.Repeat([]byte{0}, 17-len(encData))...)

What happens if len(encData) is greater than 17? I am assuming nothing is appended. If that's the case, feel free to resolve this issue.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Can you please add dgryski/go-groupvarint#1 in a comment above the padding. Also, in our chat, I had asked to add a comment about the number 17s significance. So, please do that as well.

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @animesh2049 and @manishrjain)

Copy link
Contributor Author

@animesh2049 animesh2049 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @martinmr)


codec/codec.go, line 142 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

What happens if len(encData) is greater than 17? I am assuming nothing is appended. If that's the case, feel free to resolve this issue.

Yes.

@animesh2049 animesh2049 merged commit 2823be2 into master Sep 27, 2019
@animesh2049 animesh2049 deleted the animesh2049/issue_4020 branch January 14, 2020 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Panic on Windows: possible related to integer compression.
4 participants