Patchwork [3,of,3,STABLE] rust: propagate Python exception raised from index_get_parents_checked()

login
register
mail settings
Submitter Yuya Nishihara
Date Oct. 28, 2018, 1:11 p.m.
Message ID <c40ceec4d993927b6788.1540732263@mimosa>
Download mbox | patch
Permalink /patch/36283/
State Superseded
Headers show

Comments

Yuya Nishihara - Oct. 28, 2018, 1:11 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1540730473 -32400
#      Sun Oct 28 21:41:13 2018 +0900
# Branch stable
# Node ID c40ceec4d993927b6788ad430dc6b4f3087d9aeb
# Parent  28a5ec244ba88ce4a46a26a32c24fa36f7597245
rust: propagate Python exception raised from index_get_parents_checked()

Spotted while testing with a bad stoprev value. The current Rust interface
can't report inner errors.
Yuya Nishihara - Oct. 29, 2018, 1:03 p.m.
On Sun, 28 Oct 2018 22:11:03 +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1540730473 -32400
> #      Sun Oct 28 21:41:13 2018 +0900
> # Branch stable
> # Node ID c40ceec4d993927b6788ad430dc6b4f3087d9aeb
> # Parent  28a5ec244ba88ce4a46a26a32c24fa36f7597245
> rust: propagate Python exception raised from index_get_parents_checked()

>  static int rustla_contains(rustlazyancestorsObject *self, PyObject *rev) {
> +	int res;
>  	if (!(PyInt_Check(rev))) {
>  		return 0;
>  	}
> -	return rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev));
> +	res = rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev));
> +	if (PyErr_Occurred())
> +		return -1;
> +	return res;

Not that a proper fix is to make AncestorsIterator functions return
Result<_, GraphError>. I can send the patch for stable if it's preferred.
Augie Fackler - Oct. 29, 2018, 8:32 p.m.
On Sun, Oct 28, 2018 at 10:11:03PM +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1540730473 -32400
> #      Sun Oct 28 21:41:13 2018 +0900
> # Branch stable
> # Node ID c40ceec4d993927b6788ad430dc6b4f3087d9aeb
> # Parent  28a5ec244ba88ce4a46a26a32c24fa36f7597245
> rust: propagate Python exception raised from index_get_parents_checked()

series LG, but I'll wait for the v2 with the cleaned up Result<(), ...>

>
> Spotted while testing with a bad stoprev value. The current Rust interface
> can't report inner errors.
>
> diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
> --- a/mercurial/cext/revlog.c
> +++ b/mercurial/cext/revlog.c
> @@ -2397,7 +2397,7 @@ static void rustla_dealloc(rustlazyances
>
>  static PyObject *rustla_next(rustlazyancestorsObject *self) {
>       int res = rustlazyancestors_next(self->iter);
> -	if (res == -1) {
> +	if (res == -1 || PyErr_Occurred()) {
>               /* Setting an explicit exception seems unnecessary
>                * as examples from Python source code (Objects/rangeobjets.c and
>                * Modules/_io/stringio.c) seem to demonstrate.
> @@ -2408,10 +2408,14 @@ static PyObject *rustla_next(rustlazyanc
>  }
>
>  static int rustla_contains(rustlazyancestorsObject *self, PyObject *rev) {
> +	int res;
>       if (!(PyInt_Check(rev))) {
>               return 0;
>       }
> -	return rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev));
> +	res = rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev));
> +	if (PyErr_Occurred())
> +		return -1;
> +	return res;
>  }
>
>  static PySequenceMethods rustla_sequence_methods = {
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Georges Racinet - Oct. 30, 2018, 2:48 p.m.
On 10/29/2018 02:03 PM, Yuya Nishihara wrote:
> On Sun, 28 Oct 2018 22:11:03 +0900, Yuya Nishihara wrote:
>> # HG changeset patch
>> # User Yuya Nishihara <yuya@tcha.org>
>> # Date 1540730473 -32400
>> #      Sun Oct 28 21:41:13 2018 +0900
>> # Branch stable
>> # Node ID c40ceec4d993927b6788ad430dc6b4f3087d9aeb
>> # Parent  28a5ec244ba88ce4a46a26a32c24fa36f7597245
>> rust: propagate Python exception raised from index_get_parents_checked()
>>  static int rustla_contains(rustlazyancestorsObject *self, PyObject *rev) {
>> +	int res;
>>  	if (!(PyInt_Check(rev))) {
>>  		return 0;
as a reminder, this one, that you don't modify also takes care of the
case of None, for which there is an explicit comment in the Python
implementation.
>>  	}
>> -	return rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev));
>> +	res = rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev));
>> +	if (PyErr_Occurred())
>> +		return -1;
>> +	return res;
> Not that a proper fix is to make AncestorsIterator functions return
> Result<_, GraphError>. I can send the patch for stable if it's preferred.
Returning Result<_, GraphError> as much as possible is my intent indeed,
but it wouldn't apply to next(), because the Iterator trait does not
allow for that, unless we change the signature to an Iterator with Item
= Result<Revision, GraphError>. I didn't dare to do that earlier, as it
seemed to be too heavy.

That being said, I'm not sure how much of these errors (bad stoprev etc)
we can get rid of from the start. The more serious case would be that of
the corrupted index, though, in which a correct initialization can still
give rise to errors later on.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Oct. 30, 2018, 11:04 p.m.
On Tue, 30 Oct 2018 15:48:43 +0100, Georges Racinet wrote:
> On 10/29/2018 02:03 PM, Yuya Nishihara wrote:
> >> -	return rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev));
> >> +	res = rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev));
> >> +	if (PyErr_Occurred())
> >> +		return -1;
> >> +	return res;
> > Not that a proper fix is to make AncestorsIterator functions return
> > Result<_, GraphError>. I can send the patch for stable if it's preferred.
> Returning Result<_, GraphError> as much as possible is my intent indeed,
> but it wouldn't apply to next(), because the Iterator trait does not
> allow for that, unless we change the signature to an Iterator with Item
> = Result<Revision, GraphError>. I didn't dare to do that earlier, as it
> seemed to be too heavy.
> 
> That being said, I'm not sure how much of these errors (bad stoprev etc)
> we can get rid of from the start. The more serious case would be that of
> the corrupted index, though, in which a correct initialization can still
> give rise to errors later on.

Yes, that's the reason we need an iterator yielding Result type. We can't
guarantee that the index content is valid.

Patch

diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
--- a/mercurial/cext/revlog.c
+++ b/mercurial/cext/revlog.c
@@ -2397,7 +2397,7 @@  static void rustla_dealloc(rustlazyances
 
 static PyObject *rustla_next(rustlazyancestorsObject *self) {
 	int res = rustlazyancestors_next(self->iter);
-	if (res == -1) {
+	if (res == -1 || PyErr_Occurred()) {
 		/* Setting an explicit exception seems unnecessary
 		 * as examples from Python source code (Objects/rangeobjets.c and
 		 * Modules/_io/stringio.c) seem to demonstrate.
@@ -2408,10 +2408,14 @@  static PyObject *rustla_next(rustlazyanc
 }
 
 static int rustla_contains(rustlazyancestorsObject *self, PyObject *rev) {
+	int res;
 	if (!(PyInt_Check(rev))) {
 		return 0;
 	}
-	return rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev));
+	res = rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev));
+	if (PyErr_Occurred())
+		return -1;
+	return res;
 }
 
 static PySequenceMethods rustla_sequence_methods = {