why ignore DeleteChildFile err

[Peiqi0714]

In func (fs *fileSystem) nonAtomicRename, the return err of DeleteChildFile is ignored, is it a bug?

[anushka567]

Hi @Peiqi0714, thanks for raising this question! It's a great observation, and I appreciate you taking the time to carefully review the code.

The error from DeleteChildFile is captured in the err variable:

err = oldParent.DeleteChildFile(
	ctx,
	oldName,
	oldObject.Generation,
	&oldObject.MetaGeneration)

Next, we have another error check for invalidateChildFileCacheIfExist:

if err := fs.invalidateChildFileCacheIfExist(oldParent, oldObject.Name); err != nil {
	return fmt.Errorf("nonAtomicRename: while invalidating cache for delete file: %w", err)
}

This if statement uses a short variable declaration, so the err variable inside this if is a new variable that only exists within the scope of this if statement( shadowing ).

Finally, the original err variable from the DeleteChildFile call is checked and returned if it's not nil:

if err != nil {
	err = fmt.Errorf("DeleteChildFile: %w", err)
	return err
}

So, to summarize, the error from DeleteChildFile is indeed handled correctly.I hope this explanation helps clarify the code's logic. Please let me know if you have any more questions!

[Peiqi0714]

@anushka567 Thanks for your answer, I get your point. But if DeleteChildFile returns err, it's not reasonable to call invalidateChildFileCacheIfExist. I think if DeleteChildFile returns err, we should return the function immediately, and no need to go to the next step.

[Peiqi0714]

@anushka567 I think it's more reasonable and clearly after my modification. @anushka567

[Peiqi0714]

@anushka567 You need to see the code of func (fs *fileSystem) Unlink

[anushka567]

Understood, your changes make sense. We'll make the modifications accordingly.

[Peiqi0714]

ok

[anushka567]

The PR for the same has been merged. The error returned in the delete operations incase of failure is appropriately surfaced to the application instead of ignored. Please note that we have appropriately handled different caches (stat, type and file) based on the status of the delete operation in non atomic rename as well as other delete flows.

Whenever the cache is deemed reusable and a network call can be avoided in case of failed delete operations, the caches are retained. In other cases, it is invalidated to prevent access to incorrect/stale data.

Thanks for highlighting the issue. Please feel free to reopen the issue in case of further queries.