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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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

Diff 25145

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()