-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
take the pinlock when updating pins #5550
Conversation
License: MIT Signed-off-by: Steven Allen <[email protected]>
The pin command doesn't use coreapi yet, so I'm not sure if we still want the previous PR, or to just switch pin command to CoreAPI (which should be quite simple) |
No but the pin update command does (as of your patch...). |
Oh, right, that got merged with the resolver cleanup stuff.. |
core/coreapi/pin.go
Outdated
@@ -67,6 +67,8 @@ func (api *PinAPI) Update(ctx context.Context, from coreiface.Path, to coreiface | |||
return err | |||
} | |||
|
|||
defer api.node.Blockstore.PinLock().Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need this lock before resolving? (which can take a while in case of /ipns/
paths, blocking other pin operations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(IIRC this is backed by RWLock, so not that bad of an issue but still)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but I was trying to match what we do in Pin. I thought was also concerned about the fact that we may download the first node and then throw it away but, on closer inspection, that's not a problem. We're keeping it in memory anyways.
I'll fix both of these.
License: MIT Signed-off-by: Steven Allen <[email protected]>
No description provided.