This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9783

upgrade: take lock only for part where it's required
ClosedPublic

Authored by pulkit on Jan 15 2021, 1:47 AM.

Details

Reviewers
marmoute
mharbison72
Group Reviewers
hg-reviewers
Commits
rHG17176f64a03d: upgrade: take lock only for part where it's required
Summary

The final config calculation code does not require a lock, only writing it back
does require one.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

pulkit created this revision.Jan 15 2021, 1:47 AM
marmoute requested changes to this revision.Jan 15 2021, 5:14 AM
marmoute added a subscriber: marmoute.

When upgrading, we need to read the requirements -with- the lock. Otherwise we can be raced by another process upgrading the requirements.

This revision now requires changes to proceed.Jan 15 2021, 5:14 AM
pulkit added a comment.Jan 15 2021, 7:07 AM
In D9783#147739, @marmoute wrote:

When upgrading, we need to read the requirements -with- the lock. Otherwise we can be raced by another process upgrading the requirements.

The requirements which we are reading here are source repository requirements. Hence the lock was no-op here. To prevent race condition, we will need to lock the source repository. Locking source repository on a command run by share sounded like a bad idea to me and hence left it.

marmoute added a comment.Jan 15 2021, 1:16 PM
In D9783#147791, @pulkit wrote:
In D9783#147739, @marmoute wrote:

When upgrading, we need to read the requirements -with- the lock. Otherwise we can be raced by another process upgrading the requirements.

The requirements which we are reading here are source repository requirements. Hence the lock was no-op here. To prevent race condition, we will need to lock the source repository. Locking source repository on a command run by share sounded like a bad idea to me and hence left it.

ah, I see.

However, we are mixing it with the current requirements, so we should re-read the current requirements under lock.

pulkit added a comment.Jan 18 2021, 10:20 AM
In D9783#147871, @marmoute wrote:
In D9783#147791, @pulkit wrote:
In D9783#147739, @marmoute wrote:

When upgrading, we need to read the requirements -with- the lock. Otherwise we can be raced by another process upgrading the requirements.

The requirements which we are reading here are source repository requirements. Hence the lock was no-op here. To prevent race condition, we will need to lock the source repository. Locking source repository on a command run by share sounded like a bad idea to me and hence left it.

ah, I see.
However, we are mixing it with the current requirements, so we should re-read the current requirements under lock.

Added D9822 for that.

marmoute accepted this revision.Jan 18 2021, 10:50 AM

Am I fine with the required fix being a follow up and such follow up now exist. So I think we are good to go.

(code could be a bit clearer, but nothing blocking)

This revision now requires review to proceed.Jan 18 2021, 10:50 AM
mharbison72 accepted this revision.Jan 18 2021, 9:08 PM
This revision is now accepted and ready to land.Jan 18 2021, 9:08 PM
pulkit added a commit: rHG17176f64a03d: upgrade: take lock only for part where it's required.Jan 18 2021, 9:12 PM
Closed by commit rHG17176f64a03d: upgrade: take lock only for part where it's required (authored by pulkit). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/upgrade.py (31 lines)

Diff 24881

mercurial/upgrade.py

) )
upgrade_op.print_post_op_messages() upgrade_op.print_post_op_messages()
def upgrade_share_to_safe(ui, hgvfs, storevfs, current_requirements): def upgrade_share_to_safe(ui, hgvfs, storevfs, current_requirements):
"""Upgrades a share to use share-safe mechanism""" """Upgrades a share to use share-safe mechanism"""
wlock = None wlock = None
try:
wlock = lockmod.trylock(ui, hgvfs, b'wlock', 0, 0)
store_requirements = localrepo._readrequires(storevfs, False) store_requirements = localrepo._readrequires(storevfs, False)
# after upgrade, store requires will be shared, so lets find # after upgrade, store requires will be shared, so lets find
# the requirements which are not present in store and # the requirements which are not present in store and
# write them to share's .hg/requires # write them to share's .hg/requires
diffrequires = current_requirements - store_requirements diffrequires = current_requirements - store_requirements
# add share-safe requirement as it will mark the share as share-safe # add share-safe requirement as it will mark the share as share-safe
diffrequires.add(requirementsmod.SHARESAFE_REQUIREMENT) diffrequires.add(requirementsmod.SHARESAFE_REQUIREMENT)
scmutil.writerequires(hgvfs, diffrequires)
current_requirements.add(requirementsmod.SHARESAFE_REQUIREMENT) current_requirements.add(requirementsmod.SHARESAFE_REQUIREMENT)
try:
wlock = lockmod.trylock(ui, hgvfs, b'wlock', 0, 0)
scmutil.writerequires(hgvfs, diffrequires)
ui.warn(_(b'repository upgraded to use share-safe mode\n')) ui.warn(_(b'repository upgraded to use share-safe mode\n'))
except error.LockError as e: except error.LockError as e:
if ui.configbool(b'experimental', b'sharesafe-auto-upgrade-fail-error'): if ui.configbool(b'experimental', b'sharesafe-auto-upgrade-fail-error'):
raise error.Abort( raise error.Abort(
_(b'failed to upgrade share, got error: %s') _(b'failed to upgrade share, got error: %s')
% stringutil.forcebytestr(e.strerror) % stringutil.forcebytestr(e.strerror)
) )
elif ui.configbool(b'experimental', b'sharesafe-warn-outdated-shares'): elif ui.configbool(b'experimental', b'sharesafe-warn-outdated-shares'):
def downgrade_share_to_non_safe( def downgrade_share_to_non_safe(
ui, ui,
hgvfs, hgvfs,
sharedvfs, sharedvfs,
current_requirements, current_requirements,
): ):
"""Downgrades a share which use share-safe to not use it""" """Downgrades a share which use share-safe to not use it"""
wlock = None wlock = None
try:
wlock = lockmod.trylock(ui, hgvfs, b'wlock', 0, 0)
source_requirements = localrepo._readrequires(sharedvfs, True) source_requirements = localrepo._readrequires(sharedvfs, True)
# we cannot be 100% sure on which requirements were present in store when # we cannot be 100% sure on which requirements were present in store when
# the source supported share-safe. However, we do know that working # the source supported share-safe. However, we do know that working
# directory requirements were not there. Hence we remove them # directory requirements were not there. Hence we remove them
source_requirements -= requirementsmod.WORKING_DIR_REQUIREMENTS source_requirements -= requirementsmod.WORKING_DIR_REQUIREMENTS
current_requirements |= source_requirements current_requirements |= source_requirements
current_requirements.remove(requirementsmod.SHARESAFE_REQUIREMENT) current_requirements.remove(requirementsmod.SHARESAFE_REQUIREMENT)
try:
wlock = lockmod.trylock(ui, hgvfs, b'wlock', 0, 0)
scmutil.writerequires(hgvfs, current_requirements) scmutil.writerequires(hgvfs, current_requirements)
ui.warn(_(b'repository downgraded to not use share-safe mode\n')) ui.warn(_(b'repository downgraded to not use share-safe mode\n'))
except error.LockError as e: except error.LockError as e:
# raise error right away because if downgrade failed, we cannot load # raise error right away because if downgrade failed, we cannot load
# the repository because it does not have complete set of requirements # the repository because it does not have complete set of requirements
raise error.Abort( raise error.Abort(
_(b'failed to downgrade share, got error: %s') _(b'failed to downgrade share, got error: %s')
% stringutil.forcebytestr(e.strerror) % stringutil.forcebytestr(e.strerror)
) )
finally: finally:
if wlock: if wlock:
wlock.release() wlock.release()