Skip to content
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

Downgrade global write lock after upgrading format #4599

Closed
wants to merge 3 commits into from

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Mar 15, 2021

Tentative fix for #4597 - is it strictly necessary to retain the lock at all after loading the global config?

This definitely needs a shiny new reftest - "all" that's required is a dummy package which calls opam var prefix as part of its build and to create a switch with that package.

@AltGr
Copy link
Member

AltGr commented Mar 16, 2021

Test should be fairly easy to add, see the short documentation and examples in legacy-local.git.

I may actually be in favor of completely reverting #4314 for now... if I know one thing about locks it's that their flow must really be kept as straight-forward as possible. I was already uncomfortable with adding with_flock_upgrade (you need to be really sure that you can't possibly hold a lock someone else might be waiting on before calling that, or at least that they can't be concerned with what you are upgrading... but since these things are all over the place...)
So the fix is probably correct, but it seems too complex to me to be really sure...

@dra27 dra27 linked an issue Mar 18, 2021 that may be closed by this pull request
@rjbou rjbou added this to PR in Progress in Opam 2.1.x via automation Mar 23, 2021
@rjbou rjbou added this to the 2.1.0~rc milestone Mar 23, 2021
@rjbou
Copy link
Collaborator

rjbou commented Mar 23, 2021

Looking at #4314, there was already a write lock upgrade, but it was located in the upgrade function. With the new on the fly mechanism, it is no more needed to always take a write lock. This temporary write upgrade lock can be reintroduced temporarily in OpamFormatUpgrade.as_necessary.

@rjbou rjbou added the PR: WIP Not for merge at this stage label Mar 23, 2021
@rjbou
Copy link
Collaborator

rjbou commented Mar 23, 2021

In fact, it shouldn't be needed to release the lock, as global_lock is only used with OpamFilename.with_flock_upgrade f that restores previous lock level after f. In previous version, it was restored to a write lock, so it should be more blocking. I need to take a closer look at these mechanism. Lock upgrading/downgrading is really touchy!

@rjbou rjbou self-assigned this Mar 23, 2021
@rjbou
Copy link
Collaborator

rjbou commented Mar 24, 2021

Superseded by #4612.
Thanks!

@rjbou rjbou closed this Mar 24, 2021
Opam 2.1.x automation moved this from PR in Progress to Done Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: WIP Not for merge at this stage
Projects
Opam 2.1.x
  
Done
Development

Successfully merging this pull request may close these issues.

Compilation of some packages hanging!
3 participants