You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@red-bean.com> on 2020/09/29 13:26:09 UTC
SEGFAULTs with Python bindings and generators
Hey, all. I'm wondering if I can get some extra eyes/brains on a
particular usage of our Python bindings.
The attached tarball contains a directory in which lives two files:
- run-test.sh - a shell script to drive the reproduction recipe
- pysvnget - a Python program that uses the bindings and a
generator-based wrapper of the FS's file content access APIs
If you explode the tarball, cd into the resulting directory, and run the
shell script, it should create a test repository and working copy within
that sandbox and start a loop. The loop will...
1. add text (a datestamp) to a single file in the working copy,
2. ensure the file is under version control,
3. commit the file, then
4. try to dump the content of the file from the repository using the
Python program.
The problem that I see when I do this is that after a few iterations of the
loop, the Python program starts to SEGFAULT.
I suspect there's some misinteraction with the APR pool subsystem at work
here -- my Python program is (intentionally) taking advantage of the
bindings' pool self-management logic. If I had to guess, I'd say that the
delayed access to the FS via the generator is causing reads from memory
that once lived in pools that have since been destroyed. Unfortunately, I
don't think I ever really understood how that magic worked in the first
place.
While this is a simple scenario where "don't do that" might seem an easy
enough response, what is represented by the Python program is
much-distilled logic that is live in production in some of The Company
Formerly Known As CollabNet's products. The generator approach exists to
keep server-side memory use constant while allowing http-based reads of
arbitrarily large versioned files. Moreover, the size and nature of the
codebase is such that I'd really prefer NOT to start manually doing pool
management (though as a last resort, it's not out of the cards).
Anything stand out as obviously wrong with my code?
-- Mike
Re: SEGFAULTs with Python bindings and generators
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Yasuhito FUTATSUKI wrote on Wed, 30 Sep 2020 01:08 +00:00:
> I also confirmed it occured in svn_fs_fs.is_packed_rev().
>
> Perhaps SvnContentProxy object also keep fsroot object until the
> stream is closed. I found a note in docstring for svn_fs_file_contents():
>
> """
> * @todo kff: I am worried about lifetime issues with this pool vs
> * the trail created farther down the call stack. Trace this function
> * to investigate...
> """
>
For avoidance of doubt, "trail" is a libsvn_fs_base concept, so the
concern described in the comment may or may not be applicable to the
svn_fs_fs__is_packed_rev() case. Furthermore, I think this comment
was written long before FSFS packing was created.
"kff" is kfogel@.
> I worry about the svn_stream_t * object returned by svn_fs_file_contents()
> depends that svn_fs_root_t * root is living.
Cheers,
Daniel
Re: SEGFAULTs with Python bindings and generators
Posted by "C. Michael Pilato" <cm...@red-bean.com>.
I was just about to say that I'd found something else that seems to work!
--- pysvnget.nopools 2020-09-29 09:34:52.233513574 -0400
> +++ pysvnget 2020-09-29 22:10:51.531129106 -0400
> @@ -11,6 +11,7 @@
> class SvnContentProxy:
> def __init__(self, fsroot, path):
> self.stream = svn.fs.file_contents(fsroot, path)
> + self.stream.__dict__['fsroot'] = fsroot
>
> def get_generator(self):
> while 1:
> @@ -25,6 +26,7 @@
> fs = svn.repos.fs(svn.repos.open(repos_path))
> peg_revision = peg_revision or svn.fs.youngest_rev(fs)
> fsroot = svn.fs.revision_root(fs, peg_revision)
> + fsroot.__dict__['fs'] = fs
> return SvnContentProxy(fsroot, path).get_generator()
> #
> #
> --------------------------------------------------------------------------
On Tue, Sep 29, 2020 at 10:05 PM Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
wrote:
> On 2020/09/30 10:08, Yasuhito FUTATSUKI wrote:
> > On 2020/09/30 8:51, C. Michael Pilato wrote:
> >> Thanks for the reply. And I see what you're trying to do there, but in
> >> practice it doesn't seem to work. The swig proxy object doesn't allow
> >> arbitrary attribute creation:
> >>
> >> AttributeError: You cannot add instance attributes to
> <libsvn.fs.svn_fs_t;
> >>> proxy of <Swig Object of type 'svn_fs_t *' at 0x7f83bd4be420> >
> >>
> >
> > Ah, I'm sorry you are right.
> >
> >> I tried to work around this using this construct:
> >>
> >> - fs.repos = repos
> >>> + fs.__dict__['repos'] = repos
> >>
> >>
> >> ...and that avoids the AttributeError, but alas the code still
> SEGFAULTs.
> >> I'm attaching a(n edited for readability) log from a gdb session.
> >
> > I also confirmed it occured in svn_fs_fs.is_packed_rev().
> >
> > Perhaps SvnContentProxy object also keep fsroot object until the
> > stream is closed. I found a note in docstring for svn_fs_file_contents():
>
> It doesn't suffice. It seems fsroot is also depends on fs is living.
>
> With following patch, I confirmed that it doesn't cause SEGV, at least
> revision 200 (FreeBSD 12.1, Python 3.7.8).
> [[[
> --- pysvnget.orig 2020-09-29 22:00:40.000000000 +0900
> +++ pysvnget 2020-09-30 11:02:19.300420000 +0900
> @@ -11,6 +11,7 @@ import svn.core, svn.repos, svn.fs
> class SvnContentProxy:
> def __init__(self, fsroot, path):
> self.stream = svn.fs.file_contents(fsroot, path)
> + self.root = fsroot
>
> def get_generator(self):
> while 1:
> @@ -20,11 +21,15 @@ class SvnContentProxy:
> break
> yield chunk
> svn.core.svn_stream_close(self.stream)
> + svn.root = None
>
> def get_generator(repos_path, peg_revision, path):
> - fs = svn.repos.fs(svn.repos.open(repos_path))
> + repos = svn.repos.open(repos_path)
> + fs = svn.repos.fs(repos)
> + fs.__dict__['repos'] = repos
> peg_revision = peg_revision or svn.fs.youngest_rev(fs)
> fsroot = svn.fs.revision_root(fs, peg_revision)
> + fsroot.__dict__['fs'] = fs
> return SvnContentProxy(fsroot, path).get_generator()
> #
> #
> --------------------------------------------------------------------------
> ]]]
>
>
> >
> > """
> > * @todo kff: I am worried about lifetime issues with this pool vs
> > * the trail created farther down the call stack. Trace this function
> > * to investigate...
> >
> > """
> >
> > I worry about the svn_stream_t * object returned by
> svn_fs_file_contents()
> > depends that svn_fs_root_t * root is living.
> >
> > Cheers,
> >
>
> Cheers,
> --
> Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
>
Re: SEGFAULTs with Python bindings and generators
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/09/30 10:08, Yasuhito FUTATSUKI wrote:
> On 2020/09/30 8:51, C. Michael Pilato wrote:
>> Thanks for the reply. And I see what you're trying to do there, but in
>> practice it doesn't seem to work. The swig proxy object doesn't allow
>> arbitrary attribute creation:
>>
>> AttributeError: You cannot add instance attributes to <libsvn.fs.svn_fs_t;
>>> proxy of <Swig Object of type 'svn_fs_t *' at 0x7f83bd4be420> >
>>
>
> Ah, I'm sorry you are right.
>
>> I tried to work around this using this construct:
>>
>> - fs.repos = repos
>>> + fs.__dict__['repos'] = repos
>>
>>
>> ...and that avoids the AttributeError, but alas the code still SEGFAULTs.
>> I'm attaching a(n edited for readability) log from a gdb session.
>
> I also confirmed it occured in svn_fs_fs.is_packed_rev().
>
> Perhaps SvnContentProxy object also keep fsroot object until the
> stream is closed. I found a note in docstring for svn_fs_file_contents():
It doesn't suffice. It seems fsroot is also depends on fs is living.
With following patch, I confirmed that it doesn't cause SEGV, at least
revision 200 (FreeBSD 12.1, Python 3.7.8).
[[[
--- pysvnget.orig 2020-09-29 22:00:40.000000000 +0900
+++ pysvnget 2020-09-30 11:02:19.300420000 +0900
@@ -11,6 +11,7 @@ import svn.core, svn.repos, svn.fs
class SvnContentProxy:
def __init__(self, fsroot, path):
self.stream = svn.fs.file_contents(fsroot, path)
+ self.root = fsroot
def get_generator(self):
while 1:
@@ -20,11 +21,15 @@ class SvnContentProxy:
break
yield chunk
svn.core.svn_stream_close(self.stream)
+ svn.root = None
def get_generator(repos_path, peg_revision, path):
- fs = svn.repos.fs(svn.repos.open(repos_path))
+ repos = svn.repos.open(repos_path)
+ fs = svn.repos.fs(repos)
+ fs.__dict__['repos'] = repos
peg_revision = peg_revision or svn.fs.youngest_rev(fs)
fsroot = svn.fs.revision_root(fs, peg_revision)
+ fsroot.__dict__['fs'] = fs
return SvnContentProxy(fsroot, path).get_generator()
#
# --------------------------------------------------------------------------
]]]
>
> """
> * @todo kff: I am worried about lifetime issues with this pool vs
> * the trail created farther down the call stack. Trace this function
> * to investigate...
>
> """
>
> I worry about the svn_stream_t * object returned by svn_fs_file_contents()
> depends that svn_fs_root_t * root is living.
>
> Cheers,
>
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: SEGFAULTs with Python bindings and generators
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/09/30 8:51, C. Michael Pilato wrote:
> Thanks for the reply. And I see what you're trying to do there, but in
> practice it doesn't seem to work. The swig proxy object doesn't allow
> arbitrary attribute creation:
>
> AttributeError: You cannot add instance attributes to <libsvn.fs.svn_fs_t;
>> proxy of <Swig Object of type 'svn_fs_t *' at 0x7f83bd4be420> >
>
Ah, I'm sorry you are right.
> I tried to work around this using this construct:
>
> - fs.repos = repos
>> + fs.__dict__['repos'] = repos
>
>
> ...and that avoids the AttributeError, but alas the code still SEGFAULTs.
> I'm attaching a(n edited for readability) log from a gdb session.
I also confirmed it occured in svn_fs_fs.is_packed_rev().
Perhaps SvnContentProxy object also keep fsroot object until the
stream is closed. I found a note in docstring for svn_fs_file_contents():
"""
* @todo kff: I am worried about lifetime issues with this pool vs
* the trail created farther down the call stack. Trace this function
* to investigate...
"""
I worry about the svn_stream_t * object returned by svn_fs_file_contents()
depends that svn_fs_root_t * root is living.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: SEGFAULTs with Python bindings and generators
Posted by "C. Michael Pilato" <cm...@red-bean.com>.
Thanks for the reply. And I see what you're trying to do there, but in
practice it doesn't seem to work. The swig proxy object doesn't allow
arbitrary attribute creation:
AttributeError: You cannot add instance attributes to <libsvn.fs.svn_fs_t;
> proxy of <Swig Object of type 'svn_fs_t *' at 0x7f83bd4be420> >
I tried to work around this using this construct:
- fs.repos = repos
> + fs.__dict__['repos'] = repos
...and that avoids the AttributeError, but alas the code still SEGFAULTs.
I'm attaching a(n edited for readability) log from a gdb session.
-- Mike
On Tue, Sep 29, 2020 at 4:01 PM Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
wrote:
> On 2020/09/29 22:35, C. Michael Pilato wrote:
> > Sorry for the self-reply, but I thought I'd note that if I tweak
> > pysvnget thusly, the SEGFAULTs stop:
> >
> > --- pysvnget 2020-09-29 09:34:07.918002584 -0400
> >> +++ pysvnget.pools 2020-09-29 09:33:54.278153037 -0400
> >> @@ -21,17 +21,17 @@
> >> yield chunk
> >> svn.core.svn_stream_close(self.stream)
> >>
> >> -def get_generator(repos_path, peg_revision, path):
> >> - fs = svn.repos.fs(svn.repos.open(repos_path))
> >> +def get_generator(repos_path, peg_revision, path, pool):
> >> + fs = svn.repos.fs(svn.repos.open(repos_path, pool))
>
> The pool used by fs comes from temporary svn_repos_t *repos object.
> However, svn.repos.fs wrapper function doesn't have a special
> treatment about it in current implementation, so it is nothing
> to do with the pool.
>
> If you don't want to use a pool explicitly, you need to keep the
> svn_repos_t wrapper object created by svn.repos.open() while
> fs object exists, like this.
>
> [[[
> --- pysvnget.orig 2020-09-29 22:00:40.000000000 +0900
> +++ pysvnget 2020-09-30 04:41:33.419721000 +0900
> @@ -22,7 +22,9 @@
> svn.core.svn_stream_close(self.stream)
>
> def get_generator(repos_path, peg_revision, path):
> - fs = svn.repos.fs(svn.repos.open(repos_path))
> + repos = svn.repos.open(repos_path)
> + fs = svn.repos.fs(repos)
> + fs.repos = repos
> peg_revision = peg_revision or svn.fs.youngest_rev(fs)
> fsroot = svn.fs.revision_root(fs, peg_revision)
> return SvnContentProxy(fsroot, path).get_generator()
> ]]]
>
> Cheers,
> --
> Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
>
Re: SEGFAULTs with Python bindings and generators
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/09/29 22:35, C. Michael Pilato wrote:
> Sorry for the self-reply, but I thought I'd note that if I tweak
> pysvnget thusly, the SEGFAULTs stop:
>
> --- pysvnget 2020-09-29 09:34:07.918002584 -0400
>> +++ pysvnget.pools 2020-09-29 09:33:54.278153037 -0400
>> @@ -21,17 +21,17 @@
>> yield chunk
>> svn.core.svn_stream_close(self.stream)
>>
>> -def get_generator(repos_path, peg_revision, path):
>> - fs = svn.repos.fs(svn.repos.open(repos_path))
>> +def get_generator(repos_path, peg_revision, path, pool):
>> + fs = svn.repos.fs(svn.repos.open(repos_path, pool))
The pool used by fs comes from temporary svn_repos_t *repos object.
However, svn.repos.fs wrapper function doesn't have a special
treatment about it in current implementation, so it is nothing
to do with the pool.
If you don't want to use a pool explicitly, you need to keep the
svn_repos_t wrapper object created by svn.repos.open() while
fs object exists, like this.
[[[
--- pysvnget.orig 2020-09-29 22:00:40.000000000 +0900
+++ pysvnget 2020-09-30 04:41:33.419721000 +0900
@@ -22,7 +22,9 @@
svn.core.svn_stream_close(self.stream)
def get_generator(repos_path, peg_revision, path):
- fs = svn.repos.fs(svn.repos.open(repos_path))
+ repos = svn.repos.open(repos_path)
+ fs = svn.repos.fs(repos)
+ fs.repos = repos
peg_revision = peg_revision or svn.fs.youngest_rev(fs)
fsroot = svn.fs.revision_root(fs, peg_revision)
return SvnContentProxy(fsroot, path).get_generator()
]]]
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: SEGFAULTs with Python bindings and generators
Posted by "C. Michael Pilato" <cm...@red-bean.com>.
Sorry for the self-reply, but I thought I'd note that if I tweak
pysvnget thusly, the SEGFAULTs stop:
--- pysvnget 2020-09-29 09:34:07.918002584 -0400
> +++ pysvnget.pools 2020-09-29 09:33:54.278153037 -0400
> @@ -21,17 +21,17 @@
> yield chunk
> svn.core.svn_stream_close(self.stream)
>
> -def get_generator(repos_path, peg_revision, path):
> - fs = svn.repos.fs(svn.repos.open(repos_path))
> +def get_generator(repos_path, peg_revision, path, pool):
> + fs = svn.repos.fs(svn.repos.open(repos_path, pool))
> peg_revision = peg_revision or svn.fs.youngest_rev(fs)
> fsroot = svn.fs.revision_root(fs, peg_revision)
> return SvnContentProxy(fsroot, path).get_generator()
> #
> #
> --------------------------------------------------------------------------
> -
> +pool = svn.core.svn_pool_create()
> if len(sys.argv) < 3:
> sys.stderr.write("Usage: REPOS-PATH PATH-IN-REPOS [PEGREV]\n")
> sys.exit(1)
> peg_revision = len(sys.argv) > 3 and int(sys.argv[3]) or None
> -generator = get_generator(sys.argv[1], peg_revision, sys.argv[2])
> +generator = get_generator(sys.argv[1], peg_revision, sys.argv[2], pool)
> print(b''.join(generator).decode('utf-8'))
On Tue, Sep 29, 2020 at 9:26 AM C. Michael Pilato <cm...@red-bean.com>
wrote:
> Hey, all. I'm wondering if I can get some extra eyes/brains on a
> particular usage of our Python bindings.
>
> The attached tarball contains a directory in which lives two files:
>
> - run-test.sh - a shell script to drive the reproduction recipe
> - pysvnget - a Python program that uses the bindings and a
> generator-based wrapper of the FS's file content access APIs
>
> If you explode the tarball, cd into the resulting directory, and run the
> shell script, it should create a test repository and working copy within
> that sandbox and start a loop. The loop will...
>
> 1. add text (a datestamp) to a single file in the working copy,
> 2. ensure the file is under version control,
> 3. commit the file, then
> 4. try to dump the content of the file from the repository using the
> Python program.
>
> The problem that I see when I do this is that after a few iterations of
> the loop, the Python program starts to SEGFAULT.
>
> I suspect there's some misinteraction with the APR pool subsystem at work
> here -- my Python program is (intentionally) taking advantage of the
> bindings' pool self-management logic. If I had to guess, I'd say that the
> delayed access to the FS via the generator is causing reads from memory
> that once lived in pools that have since been destroyed. Unfortunately, I
> don't think I ever really understood how that magic worked in the first
> place.
>
> While this is a simple scenario where "don't do that" might seem an easy
> enough response, what is represented by the Python program is
> much-distilled logic that is live in production in some of The Company
> Formerly Known As CollabNet's products. The generator approach exists to
> keep server-side memory use constant while allowing http-based reads of
> arbitrarily large versioned files. Moreover, the size and nature of the
> codebase is such that I'd really prefer NOT to start manually doing pool
> management (though as a last resort, it's not out of the cards).
>
> Anything stand out as obviously wrong with my code?
>
> -- Mike
>