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.

Sign up for GitHub

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

Jump to bottom

Patch to new recent bindnode PR seems to cause panic in go-graphsync #377

Closed
hannahhoward opened this issue Mar 7, 2022 · 2 comments · Fixed by #379
Closed

Patch to new recent bindnode PR seems to cause panic in go-graphsync #377

hannahhoward opened this issue Mar 7, 2022 · 2 comments · Fixed by #379

Comments

@hannahhoward
Copy link
Collaborator

After updating go-graphsync to go-ipld-prime master, I experience the following panic:

=== RUN   TestGraphsyncRoundTripIgnoreCids
panic: interface conversion: basicnode.plainList is not datamodel.Node: missing method Length [recovered]
	panic: interface conversion: basicnode.plainList is not datamodel.Node: missing method Length [recovered]
	panic: interface conversion: basicnode.plainList is not datamodel.Node: missing method Length

goroutine 1865 [running]:
go.opentelemetry.io/otel/sdk/trace.(*recordingSpan).End(0x140003f6900, {0x0, 0x0, 0x0})
	/Users/hannah/projects/go/pkg/mod/go.opentelemetry.io/otel/[email protected]/trace/span.go:283 +0x844
panic({0x1054b1b00, 0x140033403c0})
	/opt/homebrew/opt/go/libexec/src/runtime/panic.go:1038 +0x21c
go.opentelemetry.io/otel/sdk/trace.(*recordingSpan).End(0x140003f6a80, {0x0, 0x0, 0x0})
	/Users/hannah/projects/go/pkg/mod/go.opentelemetry.io/otel/[email protected]/trace/span.go:283 +0x844
panic({0x1054b1b00, 0x140033403c0})
	/opt/homebrew/opt/go/libexec/src/runtime/panic.go:1038 +0x21c
github.com/ipld/go-ipld-prime/node/bindnode.(*_mapIterator).Next(0x1400334e200)
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/node/bindnode/node.go:1124 +0x3cc
github.com/ipld/go-ipld-prime/node/bindnode.(*_mapIteratorRepr).Next(0x1400334e200)
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/node/bindnode/repr.go:253 +0x28
github.com/ipld/go-ipld-prime/codec/dagcbor.marshalMap({0x1055d1538, 0x14003340300}, 0x14000270230, {0x1055a9240, 0x1400321a280}, {0x1, 0x2})
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/codec/dagcbor/marshal.go:178 +0x380
github.com/ipld/go-ipld-prime/codec/dagcbor.marshal({0x1055d1538, 0x14003340300}, 0x14000270230, {0x1055a9240, 0x1400321a280}, {0x1, 0x2})
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/codec/dagcbor/marshal.go:69 +0x56c
github.com/ipld/go-ipld-prime/codec/dagcbor.marshalMap({0x1055d1538, 0x14003340240}, 0x14000270230, {0x1055a9240, 0x1400321a280}, {0x1, 0x2})
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/codec/dagcbor/marshal.go:214 +0x6cc
github.com/ipld/go-ipld-prime/codec/dagcbor.marshal({0x1055d1538, 0x14003340240}, 0x14000270230, {0x1055a9240, 0x1400321a280}, {0x1, 0x2})
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/codec/dagcbor/marshal.go:69 +0x56c
github.com/ipld/go-ipld-prime/codec/dagcbor.marshal({0x1055d1538, 0x14003340210}, 0x14000270230, {0x1055a9240, 0x1400321a280}, {0x1, 0x2})
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/codec/dagcbor/marshal.go:84 +0x770
github.com/ipld/go-ipld-prime/codec/dagcbor.marshalMap({0x1055d1538, 0x140033401e0}, 0x14000270230, {0x1055a9240, 0x1400321a280}, {0x1, 0x2})
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/codec/dagcbor/marshal.go:214 +0x6cc
github.com/ipld/go-ipld-prime/codec/dagcbor.marshal({0x1055d1538, 0x140033401e0}, 0x14000270230, {0x1055a9240, 0x1400321a280}, {0x1, 0x2})
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/codec/dagcbor/marshal.go:69 +0x56c
github.com/ipld/go-ipld-prime/codec/dagcbor.marshalMap({0x1055d1538, 0x140033401b0}, 0x14000270230, {0x1055a9240, 0x1400321a280}, {0x1, 0x2})
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/codec/dagcbor/marshal.go:214 +0x6cc
github.com/ipld/go-ipld-prime/codec/dagcbor.marshal({0x1055d1538, 0x140033401b0}, 0x14000270230, {0x1055a9240, 0x1400321a280}, {0x1, 0x2})
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/codec/dagcbor/marshal.go:69 +0x56c
github.com/ipld/go-ipld-prime/codec/dagcbor.Marshal(...)
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/codec/dagcbor/marshal.go:57
github.com/ipld/go-ipld-prime/codec/dagcbor.EncodeOptions.Encode({0x1, 0x2}, {0x1055d1538, 0x140033401b0}, {0x1055a85c0, 0x14003340180})
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/codec/dagcbor/marshal.go:45 +0x284
github.com/ipld/go-ipld-prime/codec/dagcbor.Encode(...)
	/Users/hannah/projects/go/pkg/mod/github.com/ipld/[email protected]/codec/dagcbor/multicodec.go:47
github.com/ipfs/go-graphsync/message/v2.(*MessageHandler).ToNet(0x105daba40, {0x14003427870, 0xb}, {0x140033147e0, 0x14003314990, 0x14003314810}, {0x12d218120, 0x14003355940})
	/Users/hannah/projects/go/src/github.com/ipfs/go-graphsync/message/v2/message.go:142 +0x11c
github.com/ipfs/go-graphsync/network.msgToStream({0x1055bbb78, 0x1400334e680}, {0x1055cebc8, 0x14003355940}, 0x14003355c80, {0x140033147e0, 0x14003314990, 0x14003314810}, 0x8bb2c97000)
	/Users/hannah/projects/go/src/github.com/ipfs/go-graphsync/network/libp2p_impl.go:129 +0x454
github.com/ipfs/go-graphsync/network.(*streamMessageSender).SendMsg(0x14003355960, {0x1055bbb78, 0x1400334e680}, {0x140033147e0, 0x14003314990, 0x14003314810})
	/Users/hannah/projects/go/src/github.com/ipfs/go-graphsync/network/libp2p_impl.go:114 +0x6c
github.com/ipfs/go-graphsync/messagequeue.(*MessageQueue).attemptSendAndRecovery(0x14000200300, {0x140033147e0, 0x14003314990, 0x14003314810}, {{0x1055bbc20, 0x140033147b0}, {0x14003314960, 0x140033149c0}, 0x0, 0x0, ...})
	/Users/hannah/projects/go/src/github.com/ipfs/go-graphsync/messagequeue/messagequeue.go:304 +0x64
github.com/ipfs/go-graphsync/messagequeue.(*MessageQueue).sendMessage(0x14000200300)
	/Users/hannah/projects/go/src/github.com/ipfs/go-graphsync/messagequeue/messagequeue.go:252 +0x874
github.com/ipfs/go-graphsync/messagequeue.(*MessageQueue).runQueue(0x14000200300)
	/Users/hannah/projects/go/src/github.com/ipfs/go-graphsync/messagequeue/messagequeue.go:159 +0x70
created by github.com/ipfs/go-graphsync/messagequeue.(*MessageQueue).Startup
	/Users/hannah/projects/go/src/github.com/ipfs/go-graphsync/messagequeue/messagequeue.go:142 +0x3c
FAIL	github.com/ipfs/go-graphsync/impl	0.739s
FAIL

it appears the latest code for the line immediately about the panic (https://github.com/ipld/go-ipld-prime/blob/master/node/bindnode/node.go#L1124) was introduced by #371 .
@rvagg
Copy link
Member

rvagg commented Mar 7, 2022

uhh, yeah, cause some of them need to be pointers still, like the basicnode plain types .. I had a bit of a play seeing if I could short-circuit this if the type implements datamodel.Node but I'm just making a bigger mess of this code. @mvdan you'd better think about how to do this.

@mvdan
Copy link
Contributor

mvdan commented Mar 7, 2022

Thanks for raising this! I'm not sure how the existing tests didn't catch this already.

This was referenced Mar 7, 2022
Closed
Merged
@mvdan mvdan closed this as completed in #379 Mar 8, 2022
mvdan added a commit that referenced this issue Mar 8, 2022
@mvdan
node/bindnode: enforce pointer requirement for nullable maps
95f4357 
The implementation expected map value types in Go to be a pointer
when the IPLD schema has the value as nullable.
Unfortunately, we forgot to enforce that in verifyCompatibility,
so the user could run into weird panics:

	panic: interface conversion: basicnode.plainList is not datamodel.Node: missing method Length

The added test case in TestPrototypePointerCombinations reproduces the
panic, but is commented out, because we're fixing the check logic to
instead have bindnode panic upfront with a helpful message.

Note that I understand why the user tried to avoid the pointer to Node.
Since Node is an interface, it is already nilable, so bindnode can store
a nil value in it directly without the need for a pointer.
We do not support that right now, but soon will; #378 now tracks that.

Fixes #377.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

node/bindnode: enforce pointer requirement for nullable maps
3 participants
@rvagg @hannahhoward @mvdan