[WIP] Make the Node methods on ProtoNode thread-safe #4784
Conversation
|
@Stebalien Could you take a quick look at the last commit? It's still a WIP but just to check if I'm going in the right direction. The idea is to encapsulate the access to the node's cache, that way adding the |
merkledag/node.go
Outdated
| // different parts of the system. As cid is the hash of the encoded | ||
| // they are closely tied together and this code should reflect that. | ||
| // If encoded is modified the cid is invalid (and should be set to nil). | ||
| cachedCID *cid.Cid |
There was a problem hiding this comment.
FYI, you can do this by creating an embedded struct:
type ProtoNode struct {
// ...
cached struct {
once sync.Once
cid *cid.Cid
bytes []byte
}
}
merkledag/node.go
Outdated
| @@ -131,7 +133,7 @@ func (n *ProtoNode) AddRawLink(name string, l *ipld.Link) error { | |||
|
|
|||
| // RemoveNodeLink removes a link on this node by the given name. | |||
| func (n *ProtoNode) RemoveNodeLink(name string) error { | |||
| n.encoded = nil | |||
| n.invalidateEncoding() | |||
There was a problem hiding this comment.
Nit: we're not invalidating the "encoding", just the "encoded" or cached object (I'd call it invalidateCache.
Encapsulated encoding access through `setEncoding` function. Make `Marshal` function private (`marshal`) to follow the `unmarshal` (private) - `DecodeProtobuf` (public) pattern. Remove CID update from `EncodeProtobuf` function, now it's only updated in its getter `Cid`. License: MIT Signed-off-by: Lucas Molas <schomatis@gmail.com>
The only time EncodeProtobuf was being called with force=true was in the DagModifier functions, which are now modified to copy the node (invalidating its cache) before modifying it (so the call to EncodeProtobuf will always rework the encoded cache value). License: MIT Signed-off-by: Lucas Molas <schomatis@gmail.com>
License: MIT Signed-off-by: Lucas Molas <schomatis@gmail.com>
|
@Stebalien Thanks for the previous review, it made much sense to encapsulate all the cache in a single structure. I'm trying to add the In case there is an error during the marshaling inside |
|
Closing as most of the code has been moved elsewhere, there may still be value here to inspire a similar PR in |
This is a work in progress, not ready to be merged.
Test of thread-safety of the
ProtoNodeimplementation of theNodeinterface (#3991). TheStat()method does an in-place sort that can result in an unordered link slice (giving an unexpected hash/CID).Closes #3991.