move icon indicating copy to clipboard operation
move copied to clipboard

[14/x][move-package/lock] Clean-up: Avoid explicit PackageLock::unlock

Open amnn opened this issue 2 years ago • 1 comments

Remove the explicit unlock function and rely on the drop handler always. The only semantic difference is the order in which the locks are unlocked, but this should not be an issue (from a deadlock perspective), because the order in which they are locked is the always the same.

Motivated by how easy (especially when using the ?/try syntax) it is to accidentally skip the explicit calls to PackageLock::unlock. The reason why this still works is that the guards that it contains automatically unlock when they are dropped, so PackageLock's drop handler should do the right thing, without need for an explicit unlock call.

Test Plan

move/language/tools/move-package$ cargo nextest

Stack

  • #741
  • #745
  • #753
  • #754
  • #759
  • #760
  • #765
  • #784
  • #785
  • #786
  • #787
  • #788
  • #789

amnn avatar Jan 02 '23 18:01 amnn

Hi @wrwg, although the wording/structure of some of the error messages is different, these changes are otherwise backwards compatible with the existing way resolution works (including package hooks). That was something we clarified when the RFC was tabled at the community sync, but I grant you that that was a while ago (mid-November last year).

At the time I think you mentioned you would pass the proposal on to @gregnazario for feedback. We haven't heard back yet on that front, but I think the best way to make sure the details that are most relevant to you are looked into is for myself and Greg (or anyone from Aptos who is interested -- I have been adding @vgao1996 as well since he expressed some interest in building on top of this work to speed up fetching) to meet and go over the structure of the stack at a high level, so I can point out the PRs that could use the most scrutiny on that front, which are:

  • #745
  • #760
  • #765
  • #786
  • #789

I won't land those PRs without making sure they preserve the way Aptos integrates with package resolution (which they intend to), let's discuss at the next community sync the best people to include in that discussion.

amnn avatar Jan 02 '23 19:01 amnn