( )⚙ D5972 exchange: raise error.Abort instead of ValueError

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5972

exchange: raise error.Abort instead of ValueError
ClosedPublic

Authored by indygreg on Feb 15 2019, 2:43 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG3b0ba4575c8c: exchange: raise error.Abort instead of ValueError
Summary

Raising ValueError results in an uncaught exception and a traceback
being printed. In the context of servers, it can result in an HTTP
500 and an exception being logged in the error log.

I don't think this is proper behavior.

The bundle2 code paths have a mechanism for translating an
error.Abort into an error message reported to the clients. I
think we should use that instead.

This commit replaces some ValueError with Abort so that
servers can error more gracefully.

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

indygreg created this revision.Feb 15 2019, 2:43 PM
Closed by commit rHG3b0ba4575c8c: exchange: raise error.Abort instead of ValueError (authored by indygreg). · Explain WhyFeb 15 2019, 2:55 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/exchange.py (8 lines)
Mtests/test-lfs-serve.t (18 lines)

Diff 14107

mercurial/exchange.py

b2caps = bundle2.bundle2caps(pushop.remote) b2caps = bundle2.bundle2caps(pushop.remote)
version = '01' version = '01'
cgversions = b2caps.get('changegroup') cgversions = b2caps.get('changegroup')
if cgversions: # 3.1 and 3.2 ship with an empty value if cgversions: # 3.1 and 3.2 ship with an empty value
cgversions = [v for v in cgversions cgversions = [v for v in cgversions
if v in changegroup.supportedoutgoingversions( if v in changegroup.supportedoutgoingversions(
pushop.repo)] pushop.repo)]
if not cgversions: if not cgversions:
raise ValueError(_('no common changegroup version')) raise error.Abort(_('no common changegroup version'))
version = max(cgversions) version = max(cgversions)
cgstream = changegroup.makestream(pushop.repo, pushop.outgoing, version, cgstream = changegroup.makestream(pushop.repo, pushop.outgoing, version,
'push') 'push')
cgpart = bundler.newpart('changegroup', data=cgstream) cgpart = bundler.newpart('changegroup', data=cgstream)
if cgversions: if cgversions:
cgpart.addparam('version', version) cgpart.addparam('version', version)
if 'treemanifest' in pushop.repo.requirements: if 'treemanifest' in pushop.repo.requirements:
cgpart.addparam('treemanifest', '1') cgpart.addparam('treemanifest', '1')
return return
version = '01' version = '01'
cgversions = b2caps.get('changegroup') cgversions = b2caps.get('changegroup')
if cgversions: # 3.1 and 3.2 ship with an empty value if cgversions: # 3.1 and 3.2 ship with an empty value
cgversions = [v for v in cgversions cgversions = [v for v in cgversions
if v in changegroup.supportedoutgoingversions(repo)] if v in changegroup.supportedoutgoingversions(repo)]
if not cgversions: if not cgversions:
raise ValueError(_('no common changegroup version')) raise error.Abort(_('no common changegroup version'))
version = max(cgversions) version = max(cgversions)
outgoing = _computeoutgoing(repo, heads, common) outgoing = _computeoutgoing(repo, heads, common)
if not outgoing.missing: if not outgoing.missing:
return return
if kwargs.get(r'narrow', False): if kwargs.get(r'narrow', False):
include = sorted(filter(bool, kwargs.get(r'includepats', []))) include = sorted(filter(bool, kwargs.get(r'includepats', [])))
@getbundle2partsgenerator('bookmarks') @getbundle2partsgenerator('bookmarks')
def _getbundlebookmarkpart(bundler, repo, source, bundlecaps=None, def _getbundlebookmarkpart(bundler, repo, source, bundlecaps=None,
b2caps=None, **kwargs): b2caps=None, **kwargs):
"""add a bookmark part to the requested bundle""" """add a bookmark part to the requested bundle"""
if not kwargs.get(r'bookmarks', False): if not kwargs.get(r'bookmarks', False):
return return
if 'bookmarks' not in b2caps: if 'bookmarks' not in b2caps:
raise ValueError(_('no common bookmarks exchange method')) raise error.Abort(_('no common bookmarks exchange method'))
books = bookmod.listbinbookmarks(repo) books = bookmod.listbinbookmarks(repo)
data = bookmod.binaryencode(books) data = bookmod.binaryencode(books)
if data: if data:
bundler.newpart('bookmarks', data=data) bundler.newpart('bookmarks', data=data)
@getbundle2partsgenerator('listkeys') @getbundle2partsgenerator('listkeys')
def _getbundlelistkeysparts(bundler, repo, source, bundlecaps=None, def _getbundlelistkeysparts(bundler, repo, source, bundlecaps=None,
b2caps=None, **kwargs): b2caps=None, **kwargs):
bundle2.buildobsmarkerspart(bundler, markers) bundle2.buildobsmarkerspart(bundler, markers)
@getbundle2partsgenerator('phases') @getbundle2partsgenerator('phases')
def _getbundlephasespart(bundler, repo, source, bundlecaps=None, def _getbundlephasespart(bundler, repo, source, bundlecaps=None,
b2caps=None, heads=None, **kwargs): b2caps=None, heads=None, **kwargs):
"""add phase heads part to the requested bundle""" """add phase heads part to the requested bundle"""
if kwargs.get(r'phases', False): if kwargs.get(r'phases', False):
if not 'heads' in b2caps.get('phases'): if not 'heads' in b2caps.get('phases'):
raise ValueError(_('no common phases exchange method')) raise error.Abort(_('no common phases exchange method'))
if heads is None: if heads is None:
heads = repo.heads() heads = repo.heads()
headsbyphase = collections.defaultdict(set) headsbyphase = collections.defaultdict(set)
if repo.publishing(): if repo.publishing():
headsbyphase[phases.public] = heads headsbyphase[phases.public] = heads
else: else:
# find the appropriate heads to move # find the appropriate heads to move

tests/test-lfs-serve.t

> opts[b'manifest'] = False > opts[b'manifest'] = False
> opts[b'dir'] = False > opts[b'dir'] = False
> rl = cmdutil.openrevlog(repo, b'debugprocessors', file_, opts) > rl = cmdutil.openrevlog(repo, b'debugprocessors', file_, opts)
> for flag, proc in rl._flagprocessors.items(): > for flag, proc in rl._flagprocessors.items():
> ui.status(b"registered processor '%#x'\n" % (flag)) > ui.status(b"registered processor '%#x'\n" % (flag))
> EOF > EOF
Skip the experimental.changegroup3=True config. Failure to agree on this comes Skip the experimental.changegroup3=True config. Failure to agree on this comes
first, and causes a "ValueError: no common changegroup version" or "abort: first, and causes an "abort: no common changegroup version" if the extension is
HTTP Error 500: Internal Server Error", if the extension is only loaded on one only loaded on one side. If that *is* enabled, the subsequent failure is "abort:
side. If that *is* enabled, the subsequent failure is "abort: missing processor missing processor for flag '0x2000'!" if the extension is only loaded on one side
for flag '0x2000'!" if the extension is only loaded on one side (possibly also (possibly also masked by the Internal Server Error message).
masked by the Internal Server Error message).
$ cat >> $HGRCPATH <<EOF $ cat >> $HGRCPATH <<EOF
> [extensions] > [extensions]
> debugprocessors = $TESTTMP/debugprocessors.py > debugprocessors = $TESTTMP/debugprocessors.py
> [experimental] > [experimental]
> lfs.disableusercache = True > lfs.disableusercache = True
> [lfs] > [lfs]
> threshold=10 > threshold=10
> [web] > [web]
[2] [2]
TODO: fail more gracefully. TODO: fail more gracefully.
$ hg init $TESTTMP/client4_pull $ hg init $TESTTMP/client4_pull
$ hg -R $TESTTMP/client4_pull pull http://localhost:$HGPORT $ hg -R $TESTTMP/client4_pull pull http://localhost:$HGPORT
pulling from http://localhost:$HGPORT/ pulling from http://localhost:$HGPORT/
requesting all changes requesting all changes
abort: HTTP Error 500: Internal Server Error remote: abort: no common changegroup version
abort: pull failed on remote
[255] [255]
$ grep 'lfs' $TESTTMP/client4_pull/.hg/requires $SERVER_REQUIRES $ grep 'lfs' $TESTTMP/client4_pull/.hg/requires $SERVER_REQUIRES
$TESTTMP/server/.hg/requires:lfs $TESTTMP/server/.hg/requires:lfs
$ hg identify http://localhost:$HGPORT $ hg identify http://localhost:$HGPORT
03b080fa9d93 03b080fa9d93
-------------------------------------------------------------------------------- --------------------------------------------------------------------------------
+++ b/lfspair2.bin Thu Jan 01 00:00:00 1970 +0000 +++ b/lfspair2.bin Thu Jan 01 00:00:00 1970 +0000
@@ -0,0 +1,1 @@ @@ -0,0 +1,1 @@
+this is an lfs file too +this is an lfs file too
#endif #endif
$ "$PYTHON" $TESTDIR/killdaemons.py $DAEMON_PIDS $ "$PYTHON" $TESTDIR/killdaemons.py $DAEMON_PIDS
#if lfsremote-on
$ cat $TESTTMP/errors.log | grep '^[A-Z]'
Traceback (most recent call last):
ValueError: no common changegroup version
#else
$ cat $TESTTMP/errors.log $ cat $TESTTMP/errors.log
#endif