[PATCH][swig-py3] Detach Python exception context in callbacks

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH][swig-py3] Detach Python exception context in callbacks

Yasuhito FUTATSUKI-2
Hi,

Jun Omae told me (with patch) that exception from Python callback in
svn_swig_py_status_func2() causes segmentation fault on Python 3.7.
(The patch have already been committed as r1551888). I think cause of
segmentation fault on py3 is the change of exception handling between
py2 and py3, support for exception context.

So I looked over callbacks in swigutil_py.c, those of return value type
aren't  svn_error_t *. I think they should be detached from callers
Python exception context because they can't report error to caller.

The patch attached fix them by inserting PyErr_Fetch() and PyErr_Restore()
save and restore Python error indicator.

(The patch in other thread textually conflict with this patch, though)

Cheers,
--
Yasuhito FUTATSUKI

detach_python_exception_patch.txt (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][swig-py3] Detach Python exception context in callbacks

Daniel Shahaf-2
Yasuhito FUTATSUKI wrote on Sun, 03 Feb 2019 20:52 +0900:
> The patch attached fix them by inserting PyErr_Fetch() and PyErr_Restore()
> save and restore Python error indicator.
>
> (The patch in other thread textually conflict with this patch, though)

I don't have an opinion on this specific patch, but regarding the
textual conflict, I don't think it is an efficient workflow for you to
work with various patches flying around.  Please feel free to commit
your patches directly to the branch for them to be reviewed post-commit,
or — if you prefer — to create one or more new branches off the swig-py3
branch and commit your patches thereto, for them to be merged back to
the swig-py3 branch once they have been reviewed.

https://subversion.apache.org/docs/community-guide/general.html#lightweight-branches
(with s/trunk/swig-py3/g)

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][swig-py3] Detach Python exception context in callbacks

Yasuhito FUTATSUKI-2
On 2/3/19 11:30 PM, Daniel Shahaf wrote:

> Yasuhito FUTATSUKI wrote on Sun, 03 Feb 2019 20:52 +0900:
>> The patch attached fix them by inserting PyErr_Fetch() and PyErr_Restore()
>> save and restore Python error indicator.
>>
>> (The patch in other thread textually conflict with this patch, though)
>
> I don't have an opinion on this specific patch, but regarding the
> textual conflict, I don't think it is an efficient workflow for you to
> work with various patches flying around.  Please feel free to commit
> your patches directly to the branch for them to be reviewed post-commit,
> or — if you prefer — to create one or more new branches off the swig-py3
> branch and commit your patches thereto, for them to be merged back to
> the swig-py3 branch once they have been reviewed.
>
> https://subversion.apache.org/docs/community-guide/general.html#lightweight-branches
> (with s/trunk/swig-py3/g)

Thank you for the guidance. Now I commited this patch as r1852967.

Then I'll back to improve the former patch.

Thanks,
--
Yasuhito FUTATSUKI


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][swig-py3] Detach Python exception context in callbacks

Daniel Shahaf-2
Yasuhito FUTATSUKI wrote on Tue, 05 Feb 2019 16:11 +0900:

> On 2/3/19 11:30 PM, Daniel Shahaf wrote:
> > Yasuhito FUTATSUKI wrote on Sun, 03 Feb 2019 20:52 +0900:
> >> The patch attached fix them by inserting PyErr_Fetch() and PyErr_Restore()
> >> save and restore Python error indicator.
> >>
> >> (The patch in other thread textually conflict with this patch, though)
> >
> > I don't have an opinion on this specific patch, but regarding the
> > textual conflict, I don't think it is an efficient workflow for you to
> > work with various patches flying around.  Please feel free to commit
> > your patches directly to the branch for them to be reviewed post-commit,
> > or — if you prefer — to create one or more new branches off the swig-py3
> > branch and commit your patches thereto, for them to be merged back to
> > the swig-py3 branch once they have been reviewed.
> >
> > https://subversion.apache.org/docs/community-guide/general.html#lightweight-branches
> > (with s/trunk/swig-py3/g)
>
> Thank you for the guidance. Now I commited this patch as r1852967.
>
> Then I'll back to improve the former patch.

Great!  Remember that you can always ask dev@ to review or advice on any
particular question, too, whether or not the related change has been committed.

Cheers,

Daniel