ipldutil: simplify state synchronization, add docs #300
Conversation
|
I've stared at the code for a while, and I think understanding the TODO bits will help unblock me to document the rest of the code. |
|
Is the intent to resolve the TODOs on this branch, or do them piecemeal in future updates? Other than that comment block that Hannah has provided some clarity on this lgtm since it doesn't look like it has material impact. |
mvdan
changed the title
|
@rvagg apologies for the late reply - the TODOs were open questions for Hannah, really. The intent was to have them during the early stages of code review so I could understand how the code worked. |
|
@hannahhoward this should be ready for review and/or testing. Please pay extra attention to how the mutex is documented and used, in particular to the |
|
Friendly bump that this is ready for review/merge :) |
The two goroutines were synchronized via two channels, awaitRequest to signal when StorageReadOpener starts, and responses to provide it with a value to unblock and return. Besides those two, there was also stateChan, to pass state around. Keep the responses channel in place, but replace awaitRequest and stateChan with a mutex and plain fields. The added docs describe how they are used and why. Note that we have to be careful when locking and unlocking the mutex. We can't unlock an unlocked mutex, nor leave the mutex locked. Each call to Lock or Unlock comes with a reasoning. parentCtx isn't used beyond the constructor, so remove the field. The visitor and chooser defaults are fallbacks, so use them as such. Use explicit struct keys for literals, making the code easier to follow. Fixes #274.
|
Should be ready for a final review+merge I think :) |
|
Seems like a flake on TestNetworkDisconnect. Does that ring a bell to either of you? I don't imagine my change is the cause. |
* feat(deps): update graphsync to 2.0 branch Provide the absolute minimum required to work with the latest version of graphsync with the 2.0 protocl. This does use a very unfortunate hack to deal with the change to extension type -- it now deserializes/serializes from basicnode.Any in order to get back to a []byte type * style(mod): tidy * refactor(message): create To/From IPLD funcs * fix(impl): add one more int conversion * feat(msg): ipld-prime messages, compat testing w/ cbor-gen * fix(msg): replace message 1_1 with 1_1prime form * chore(msg): rename struct fields, use renames for consistent encoding * fix(msg): remove through-bytes roundtrip for conversions * fix(msg): fixed upstream bindnode bug, added some temporary debugging * fix(graphsync): compare Nodes with ipld.DeepEqual * fix(deps): update ipld-prime with bindnode tuple fix * fix(msg): remove debug prints, fix imports * fix(cbor-gen): make go generate work again in old msgs * chore(deps): update go-ipld-prime to new master with tuple fix * feat(graphsync): remove peer id tracking no longer need to utilize peer id to tract graphsync requests since RequestIDs are unique * feat(deps): update to tagged go-graphsync Co-authored-by: Rod Vagg <rod@vagg.org>
(see commit message)
Fixes #274.