The add command's addAllAndPin function doesn't hold the pin lock

[Stebalien]

While the file adder does hold the pin lock while adding a single file, the outer addAllAndPin function doesn't. This means that we can GC between adding and pinning.

[schomatis]

@Stebalien Is someone working on this? I'm interested to learn more about the locking system while adding/pinning, as it's something I stumbled upon while working on #4650.

[Stebalien]

Nope. All yours!

[schomatis]

@Stebalien The exported AddFile function is just a wrapper for the internal addFile function while also holding the pin lock, and it's only used by addAllAndPin (in the ipfs add command). I think the logic of addAllAndPin should belong to the Adder and not to the ipfs add command (as the latter it's taking more responsability than it should about the add/pin logic). In this way addAllAndPin could directly use the addFile internal function while holding the pin lock all the way (including the call to PinRoot).

So the proposal would be:

1. Move addAllAndPin inside the Adder (in coreunix), replacing the call to AddFile with addFile.

2. Copy the pin lock logic from AddFile to addAllAndPin covering the calls to addFile and PinRoot.

3. (optional) Eliminate AddFile which doesn't seem to be used anywhere outside addAllAndPin (and some tests).

As side note, I'm having trouble understanding the purpose of PinLock. Its name and comment seem to imply that someone who grabs that lock will use it to pin content, but the lock (that seems more related to the GC than the pin/add functionality) is commonly used just for add operations (that don't pin while holding this lock), e.g., AddFile, AddR and AddWrapped.

[Stebalien]

Its name and comment seem to imply that someone who grabs that lock will use it to pin content, but the lock (that seems more related to the GC than the pin/add functionality)

You're right, it's a "don't GC" lock.

---

So, being able to add individual files is nice. We'll want to discuss this a bit before we get rid of that ability.

Ideally, anything added with the adder should be temporarily pinned until we call Finalize. At that point, we should remove the temporary pin add a real pin if adder.Pin was set. Unfortunately, we don't have a temporary pin system at the moment (although we desperately need one and it would allow us to get rid of the pin lock entirely).

@lgierth any opinions on this API?

[kjzz]

hey @schomatis , are you still doing for this?if you do not finish, maybe i can help.because i am learn more about the add/pin.So i think maybe i can have a try.

[schomatis]

Great! But we should first check with @Stebalien the status of this issue

[kjzz]

Thx a lot @schomatis. If this issue is still pending,please ping me @Stebalien .

[kjzz]

Hey @schomatis @Stebalien , i have create a pr #5506 about this issue . Could you help me review it?