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

feat: remove block decoding global registry #80

Merged
merged 1 commit into from
Jun 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 35 additions & 27 deletions coding.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,44 +2,53 @@ package format

import (
"fmt"
"sync"

blocks "github.com/ipfs/go-block-format"
)

// DecodeBlockFunc functions decode blocks into nodes.
type DecodeBlockFunc func(block blocks.Block) (Node, error)

type BlockDecoder interface {
Register(codec uint64, decoder DecodeBlockFunc)
Decode(blocks.Block) (Node, error)
}
type safeBlockDecoder struct {
// Can be replaced with an RCU if necessary.
lock sync.RWMutex
// Registry is a structure for storing mappings of multicodec IPLD codec numbers to DecodeBlockFunc functions.
//
// Registry includes no mutexing. If using Registry in a concurrent context, you must handle synchronization yourself.
// (Typically, it is recommended to do initialization earlier in a program, before fanning out goroutines;
// this avoids the need for mutexing overhead.)
//
// Multicodec indicator numbers are specified in
// https://github.com/multiformats/multicodec/blob/master/table.csv .
// You should not use indicator numbers which are not specified in that table
// (however, there is nothing in this implementation that will attempt to stop you, either).
type Registry struct {
decoders map[uint64]DecodeBlockFunc
}

func (r *Registry) ensureInit() {
if r.decoders != nil {
return
}
r.decoders = make(map[uint64]DecodeBlockFunc)
}

// Register registers decoder for all blocks with the passed codec.
//
// This will silently replace any existing registered block decoders.
func (d *safeBlockDecoder) Register(codec uint64, decoder DecodeBlockFunc) {
d.lock.Lock()
defer d.lock.Unlock()
d.decoders[codec] = decoder
func (r *Registry) Register(codec uint64, decoder DecodeBlockFunc) {
r.ensureInit()
if decoder == nil {
panic("not sensible to attempt to register a nil function")
}
r.decoders[codec] = decoder
}

func (d *safeBlockDecoder) Decode(block blocks.Block) (Node, error) {
func (r *Registry) Decode(block blocks.Block) (Node, error) {
// Short-circuit by cast if we already have a Node.
if node, ok := block.(Node); ok {
return node, nil
}

ty := block.Cid().Type()

d.lock.RLock()
decoder, ok := d.decoders[ty]
d.lock.RUnlock()
r.ensureInit()

Choose a reason for hiding this comment

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

is there a way to avoid doing this work on every usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied this from the go-ipld-prime one https://github.com/ipld/go-ipld-prime/blob/master/multicodec/registry.go. But yeah happy to change it either here, or in a patch release if this ends up being annoying for people.

Copy link
Contributor Author

@aschmahmann aschmahmann Jun 6, 2023

Choose a reason for hiding this comment

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

and a lock protecting concurrent map accesses

Same in that I copied from go-ipld-prime. The struct here used to be threadsafe and I'm fine with going back to that version too if preferred.

decoder, ok := r.decoders[ty]

if ok {
return decoder(block)
Expand All @@ -49,14 +58,13 @@ func (d *safeBlockDecoder) Decode(block blocks.Block) (Node, error) {
}
}

var DefaultBlockDecoder BlockDecoder = &safeBlockDecoder{decoders: make(map[uint64]DecodeBlockFunc)}

// Decode decodes the given block using the default BlockDecoder.
func Decode(block blocks.Block) (Node, error) {
return DefaultBlockDecoder.Decode(block)
}
// Decode decodes the given block using passed DecodeBlockFunc.
// Note: this is just a helper function, consider using the DecodeBlockFunc itself rather than this helper
func Decode(block blocks.Block, decoder DecodeBlockFunc) (Node, error) {
Copy link
Contributor

@Jorropo Jorropo May 30, 2023

Choose a reason for hiding this comment

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

I'm questioning the value of this function. If the caller has the decoder already why would it call it in here ? I would call the decoder directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm cool just deleting this given we're breaking (a very small number of) users here anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing this does is shortcut if your block is already a node which is not necessarily an obvious optimization. This makes the modifications upstream a no-brainer one line thing (import the codec + pass it) rather than a few lines of copy-paste.

Copy link
Contributor

@Jorropo Jorropo May 30, 2023

Choose a reason for hiding this comment

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

If a consumer randomly handle mixed basic blocks and deserialized ones it sound like a them issue.
What about using strong types ? 🙃

I'm fine to keep this as-is if anyone incists.

Choose a reason for hiding this comment

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

i'm okay with removing it entirely

Copy link
Contributor Author

@aschmahmann aschmahmann Jun 6, 2023

Choose a reason for hiding this comment

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

Being the person doing the bubbling (and having now gone and located a bunch of codebases to do this) I'm thinking let's just keep it and add a comment that it's a helper function. I'd like propagating updates here to be as mindless as possible, changing behavior doesn't make it mindless.

// Short-circuit by cast if we already have a Node.
if node, ok := block.(Node); ok {
return node, nil
}

// Register registers block decoders with the default BlockDecoder.
func Register(codec uint64, decoder DecodeBlockFunc) {
DefaultBlockDecoder.Register(codec, decoder)
return decoder(block)
}
54 changes: 49 additions & 5 deletions coding_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,53 @@ import (
mh "github.com/multiformats/go-multihash"
)

func init() {
Register(cid.Raw, func(b blocks.Block) (Node, error) {
func TestDecode(t *testing.T) {
decoder := func(b blocks.Block) (Node, error) {
node := &EmptyNode{}
if b.RawData() != nil || !b.Cid().Equals(node.Cid()) {
return nil, errors.New("can only decode empty blocks")
}
return node, nil
})
}

id, err := cid.Prefix{
Version: 1,
Codec: cid.Raw,
MhType: mh.ID,
MhLength: 0,
}.Sum(nil)

if err != nil {
t.Fatalf("failed to create cid: %s", err)
}

block, err := blocks.NewBlockWithCid(nil, id)
if err != nil {
t.Fatalf("failed to create empty block: %s", err)
}
node, err := Decode(block, decoder)
if err != nil {
t.Fatalf("failed to decode empty node: %s", err)
}
if !node.Cid().Equals(id) {
t.Fatalf("empty node doesn't have the right cid")
}

if _, ok := node.(*EmptyNode); !ok {
t.Fatalf("empty node doesn't have the right type")
}

}

func TestDecode(t *testing.T) {
func TestRegistryDecode(t *testing.T) {
decoder := func(b blocks.Block) (Node, error) {
node := &EmptyNode{}
if b.RawData() != nil || !b.Cid().Equals(node.Cid()) {
return nil, errors.New("can only decode empty blocks")
}
return node, nil
}

id, err := cid.Prefix{
Version: 1,
Codec: cid.Raw,
Expand All @@ -35,10 +71,18 @@ func TestDecode(t *testing.T) {
if err != nil {
t.Fatalf("failed to create empty block: %s", err)
}
node, err := Decode(block)

reg := Registry{}
_, err = reg.Decode(block)
if err == nil || err.Error() != "unrecognized object type: 85" {
t.Fatalf("expected error, got %v", err)
}
reg.Register(cid.Raw, decoder)
node, err := reg.Decode(block)
if err != nil {
t.Fatalf("failed to decode empty node: %s", err)
}

if !node.Cid().Equals(id) {
t.Fatalf("empty node doesn't have the right cid")
}
Expand Down