[lxml-dev] Test failures on Windows

I think I've mentioned the other day the fact some failures are seen when running tests for trunk on Windows. Here's the log: http://tinyurl.com/yhd4hc If you want to follow the status of the builds, you can use this page. You can also force a build from there by clicking on the builder name (same line as 'changes' header): http://tinyurl.com/yykhz7 -- Sidnei da Silva http://www.enfoldsystems.com

Hi, Sidnei da Silva wrote:
Thanks. Most of those are the usual Windows bugs that you can't delete an open file. I fixed some and just silenced the remaining two - shouldn't be too much of a problem if tiny temporary files are not deleted after running the test cases (which is a rare enough event anyway...) There's one problem left, though, and I don't have any idea where it might come from. ====================================================================== FAIL: test_xslt_parameters (lxml.tests.test_xslt.ETreeXSLTTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "C:\pybots\slave\trunk.dasilva-x86\build\lib\unittest.py", line 260, in run testMethod() File "C:\pybots\slave\python-tool\lxml-trunk\src\lxml\tests\test_xslt.py", line 210, in test_xslt_parameters st.apply, tree) File "C:\pybots\slave\trunk.dasilva-x86\build\lib\unittest.py", line 326, in failUnlessRaises raise self.failureException, "%s not raised" % excName AssertionError: XSLTApplyError not raised ---------------------------------------------------------------------- Test case being: ---------------------------------------------------------------------- def test_xslt_parameters(self): tree = self.parse('<a><b>B</b><c>C</c></a>') style = self.parse('''\ <xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform"> <xsl:template match="*" /> <xsl:template match="/"> <foo><xsl:value-of select="$bar" /></foo> </xsl:template> </xsl:stylesheet>''') st = etree.XSLT(style) res = st.apply(tree, bar="'Bar'") self.assertEquals('''\ <?xml version="1.0"?> <foo>Bar</foo> ''', st.tostring(res)) # apply without needed parameter will lead to XSLTApplyError self.assertRaises(etree.XSLTApplyError, st.apply, tree) ---------------------------------------------------------------------- Apparently, the comment is wrong here... I'll have to find some time to look into this, unless someone has an idea? Could anyone test this under Windows and maybe figure out what happens here? Sometimes this means that an unexpected exception is raised instead - or none at all? Stefan

On Fri, Oct 27, 2006 at 10:55:11PM +0200, Stefan Behnel wrote: | Thanks. Most of those are the usual Windows bugs that you can't delete an open | file. I fixed some and just silenced the remaining two - shouldn't be too much | of a problem if tiny temporary files are not deleted after running the test | cases (which is a rare enough event anyway...) Yeah, as long as they are tiny :) I will be running the tests constantly on that box. Maybe I can setup some task to clean up $TMP. | There's one problem left, though, and I don't have any idea where it might | come from. | | ====================================================================== | FAIL: test_xslt_parameters (lxml.tests.test_xslt.ETreeXSLTTestCase) | ---------------------------------------------------------------------- | Traceback (most recent call last): | File "C:\pybots\slave\trunk.dasilva-x86\build\lib\unittest.py", line 260, in run | testMethod() | File "C:\pybots\slave\python-tool\lxml-trunk\src\lxml\tests\test_xslt.py", | line 210, in test_xslt_parameters | st.apply, tree) | File "C:\pybots\slave\trunk.dasilva-x86\build\lib\unittest.py", line 326, in | failUnlessRaises | raise self.failureException, "%s not raised" % excName | AssertionError: XSLTApplyError not raised | ---------------------------------------------------------------------- | | Test case being: | | ---------------------------------------------------------------------- | def test_xslt_parameters(self): | tree = self.parse('<a><b>B</b><c>C</c></a>') | style = self.parse('''\ | <xsl:stylesheet version="1.0" | xmlns:xsl="http://www.w3.org/1999/XSL/Transform"> | <xsl:template match="*" /> | <xsl:template match="/"> | <foo><xsl:value-of select="$bar" /></foo> | </xsl:template> | </xsl:stylesheet>''') | | st = etree.XSLT(style) | res = st.apply(tree, bar="'Bar'") | self.assertEquals('''\ | <?xml version="1.0"?> | <foo>Bar</foo> | ''', | st.tostring(res)) | # apply without needed parameter will lead to XSLTApplyError | self.assertRaises(etree.XSLTApplyError, | st.apply, tree) | ---------------------------------------------------------------------- | | Apparently, the comment is wrong here... | | I'll have to find some time to look into this, unless someone has an idea? | Could anyone test this under Windows and maybe figure out what happens here? | Sometimes this means that an unexpected exception is raised instead - or none | at all? I can look at that later today. -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

| Apparently, the comment is wrong here... | | I'll have to find some time to look into this, unless someone has an idea? | Could anyone test this under Windows and maybe figure out what happens here? | Sometimes this means that an unexpected exception is raised instead - or none | at all? So here's what that line gives: (Pdb) p st.tostring(st.apply(tree)) '<?xml version="1.0"?>\n<foo/>\n' Looks like it just assumed the parameter was empty or something. -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi Stefan, On Fri, Oct 27, 2006 at 09:06:59PM -0300, Sidnei da Silva wrote: | | Apparently, the comment is wrong here... | | | | I'll have to find some time to look into this, unless someone has an idea? | | Could anyone test this under Windows and maybe figure out what happens here? | | Sometimes this means that an unexpected exception is raised instead - or none | | at all? | | So here's what that line gives: | | (Pdb) p st.tostring(st.apply(tree)) | '<?xml version="1.0"?>\n<foo/>\n' | | Looks like it just assumed the parameter was empty or something. Did you had a chance to look at the XSLTApplyError not being raised? Does that test fail on Linux? Maybe it's an issue with the version of libxml2 that I'm using on the buildbot? -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi Sidnei, Sidnei da Silva wrote:
Yes, I looked into it and decided it's not critical enough to delay 1.1.2 even more.
Does that test fail on Linux?
No, not on my machine.
Maybe it's an issue with the version of libxml2 that I'm using on the buildbot?
I tried with libxml2 2.6.24 and 2.6.26. Both pass the test nicely. I also looked through the code path and couldn't find anything obvious that would behave differently on different systems. I have no idea why that test fails on the buildbot. BTW, the buildbot logs (all of them) seem to be broken currently, don't know where that comes from. Stefan

On Mon, Oct 30, 2006 at 06:28:56PM +0100, Stefan Behnel wrote: | I tried with libxml2 2.6.24 and 2.6.26. Both pass the test nicely. I also | looked through the code path and couldn't find anything obvious that would | behave differently on different systems. I have no idea why that test fails on | the buildbot. I see that you added some extra output with the version numbers to the output. http://tinyurl.com/ymbmrt | BTW, the buildbot logs (all of them) seem to be broken currently, don't know | where that comes from. Yeah, the master got out of sync, somehow. It's working again now. -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

On Fri, Oct 27, 2006 at 10:55:11PM +0200, Stefan Behnel wrote: | Thanks. Most of those are the usual Windows bugs that you can't delete an open | file. I fixed some and just silenced the remaining two - shouldn't be too much | of a problem if tiny temporary files are not deleted after running the test | cases (which is a rare enough event anyway...) Two issues here: - In Python2.4 it raises OSError instead of WindowsError. I guess that is one of the changes in Python2.5. - I believe that this might be a real bug that needs fixing. Why it might be a bug: - I looked at the source in lxml and I see that this ends up calling xmlparser.xmlCtxtReadFile, which just delegates down to libxml2. Well, somewhere in there it seems like the file is read but not closed. By trial-and-failure, I've come up with the attached patch, which fixes the failures on Windows. Someone more experienced should review this. careful-not-to-hide-the-dirt-under-the-rug'ly yours, -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi Sidnei, Sidnei da Silva wrote:
Good to know.
You got me convinced. I think that's because we are using the context reusing calls (xmlCtxt*). They require calling xmlCtxtReset afterwards to clean up both the input stack and memory resources. This is normally called automatically when using the parser context the next time (which is why there never were any enduring side effects), but waiting for that has the temporal side effect of leaving the input stream open when passing control back to the user code. Now, the problem is, running xmlCtxtReset can currently segfault in some cases, so we can't just call it carelessly. I played with it a bit to figure out in which cases it can be called, but it doesn't look like we can safely call it in every case where it would make sense. Guess I'll file a bug report on it and try to come up with a work-around... Stefan

On Sat, Oct 28, 2006 at 10:05:14AM +0200, Stefan Behnel wrote: | You got me convinced. I think that's because we are using the context reusing | calls (xmlCtxt*). They require calling xmlCtxtReset afterwards to clean up | both the input stack and memory resources. This is normally called | automatically when using the parser context the next time (which is why there | never were any enduring side effects), but waiting for that has the temporal | side effect of leaving the input stream open when passing control back to the | user code. | | Now, the problem is, running xmlCtxtReset can currently segfault in some | cases, so we can't just call it carelessly. I played with it a bit to figure | out in which cases it can be called, but it doesn't look like we can safely | call it in every case where it would make sense. Guess I'll file a bug report | on it and try to come up with a work-around... Erm.... did you look at the attached patch? It just frees ctxt->input if its not NULL. I guess you're looking for a generic fix though. -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi Sidnei, Sidnei da Silva wrote:
Not only generic. Pending open files is only a symptom here. The real problem is that none of the resources allocated for parsing is freed before you call the parser again (in which case new resources will be allocated right away). So popping the input streams fixes the windows problem, but calling xmlClearParserCtxt() after parsing would be the right thing to do - if it didn't crash. Stefan

On Sat, Oct 28, 2006 at 03:16:43PM +0200, Stefan Behnel wrote: | Not only generic. Pending open files is only a symptom here. The real problem | is that none of the resources allocated for parsing is freed before you call | the parser again (in which case new resources will be allocated right away). | | So popping the input streams fixes the windows problem, but calling | xmlClearParserCtxt() after parsing would be the right thing to do - if it | didn't crash. Thanks for the explanation. That makes a lot of sense. A question though. Is the parser context expensive to allocate? Why not use xmlFreeParserCtxt() and allocate a new one instead of xmlResetParserCtxt()? -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

On Sat, Oct 28, 2006 at 03:43:05PM +0200, Stefan Behnel wrote: | It is pretty expensive to allocate. There are some hash-table allocations | involved that are rather costly (that's why there is a function for resetting | the context). In lxml, we try to reuse context objects wherever possible. One last question, do you have a small test that can reproduce the segfault or is it just random? I would like to spend some time tracking that one down. -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi, Sidnei da Silva wrote:
No need to do that. It's in line 12837 in file parser.c. Apparently, the assumption that ctxt->spaceTab has always been initialised when calling xmlCtxtReset() is wrong. Here's my bug report: http://bugzilla.gnome.org/show_bug.cgi?id=366161 My current work around is to do the NULL check myself and to initialise it the way libxml2 normally does before calling the reset. That's what's I'd normally expect libxml2 to do... BTW, I don't know in which cases this field remains uninitialised, so if you want to add some more infos to the bug report, feel free to investigate. I only know that it definitely happens in the case where we call xmlCtxtReadFile(). It is possible that this requires previous runs of the parser, maybe even a failed run preceding the crash, can't tell... Stefan

On Sat, Oct 28, 2006 at 04:12:30PM +0200, Stefan Behnel wrote: | No need to do that. It's in line 12837 in file parser.c. Apparently, the | assumption that ctxt->spaceTab has always been initialised when calling | xmlCtxtReset() is wrong. Here's my bug report: | | http://bugzilla.gnome.org/show_bug.cgi?id=366161 | | My current work around is to do the NULL check myself and to initialise it the | way libxml2 normally does before calling the reset. That's what's I'd normally | expect libxml2 to do... Yes, that check is certainly missing. Note that htmlCtxtReset() does the check! I added that info to the bug report. | BTW, I don't know in which cases this field remains uninitialised, so if you | want to add some more infos to the bug report, feel free to investigate. I | only know that it definitely happens in the case where we call | xmlCtxtReadFile(). It is possible that this requires previous runs of the | parser, maybe even a failed run preceding the crash, can't tell... Maybe in spacePush, if memory allocation fails. That seems a bit odd though... unless you're really short on memory :) -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi Sidnei, Sidnei da Silva wrote:
I think I was on the wrong track. It's a bug in libxml2, but not the right one. The crash appears somewhere in the long doctest in api.txt, most likely in a place where errors are tested when parsing from a string. That's not what we are looking for. You said that your patch fixes the problem, are you certain about that? Because calling xmlCtxtReset() should do exactly that (and a lot more) and it doesn't seem to solve the problem - according to the buildbot. Stefan

On Sat, Oct 28, 2006 at 09:55:55PM +0200, Stefan Behnel wrote: | I think I was on the wrong track. It's a bug in libxml2, but not the right | one. The crash appears somewhere in the long doctest in api.txt, most likely | in a place where errors are tested when parsing from a string. That's not what | we are looking for. Ok... but that's no excuse for the check in HTMLparser to not be done on parser. | You said that your patch fixes the problem, are you certain about that? | Because calling xmlCtxtReset() should do exactly that (and a lot more) and it | doesn't seem to solve the problem - according to the buildbot. Yes, it does solve the problem for me on Python 2.4, ie, OSError is not raised after applying my patch. -- -e Sidnei da Silva -e Enfold Systems http://enfoldsystems.com -e Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

| | You said that your patch fixes the problem, are you certain about that? | | Because calling xmlCtxtReset() should do exactly that (and a lot more) and it | | doesn't seem to solve the problem - according to the buildbot. I see that you only call xmlClearContext if spaceTab is not NULL. Maybe that's the issue. -- -e Sidnei da Silva -e Enfold Systems http://enfoldsystems.com -e Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi Sidnei, Sidnei da Silva wrote:
No, I also tried initialising spaceTab by hand to always call reset(). No difference. So this is definitely not the problem. And it really makes me wonder how your patch can work if xmlClearParserCtxt() does not, because your code snippet is straight in there and I can't see a way it should not get executed if clear() is called. AFAICT, with the call to clear(), all input streams and memory resources should get freed after parsing, so that's all I wanted. And still the problem of a file descriptor staying open remains. I'll wait for the next buildbot run to see, but if that fails, I'll really get clueless... Note, BTW, that the error occurs in a finally block, so maybe it's already shadowing an exception? Stefan

On Sun, Oct 29, 2006 at 12:05:40AM +0200, Stefan Behnel wrote: | No, I also tried initialising spaceTab by hand to always call reset(). No | difference. So this is definitely not the problem. And it really makes me | wonder how your patch can work if xmlClearParserCtxt() does not, because your | code snippet is straight in there and I can't see a way it should not get | executed if clear() is called. | | AFAICT, with the call to clear(), all input streams and memory resources | should get freed after parsing, so that's all I wanted. And still the problem | of a file descriptor staying open remains. I'll wait for the next buildbot run | to see, but if that fails, I'll really get clueless... FWIW, I reverted my patch and did a svn up, and your changes seem to work here. No segfault or anything. The buildbot seems to be in some funny state now, maybe due to some checkin on Python 2.5/trunk. -- -e Sidnei da Silva -e Enfold Systems http://enfoldsystems.com -e Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi, Sidnei da Silva wrote:
Thanks. Most of those are the usual Windows bugs that you can't delete an open file. I fixed some and just silenced the remaining two - shouldn't be too much of a problem if tiny temporary files are not deleted after running the test cases (which is a rare enough event anyway...) There's one problem left, though, and I don't have any idea where it might come from. ====================================================================== FAIL: test_xslt_parameters (lxml.tests.test_xslt.ETreeXSLTTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "C:\pybots\slave\trunk.dasilva-x86\build\lib\unittest.py", line 260, in run testMethod() File "C:\pybots\slave\python-tool\lxml-trunk\src\lxml\tests\test_xslt.py", line 210, in test_xslt_parameters st.apply, tree) File "C:\pybots\slave\trunk.dasilva-x86\build\lib\unittest.py", line 326, in failUnlessRaises raise self.failureException, "%s not raised" % excName AssertionError: XSLTApplyError not raised ---------------------------------------------------------------------- Test case being: ---------------------------------------------------------------------- def test_xslt_parameters(self): tree = self.parse('<a><b>B</b><c>C</c></a>') style = self.parse('''\ <xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform"> <xsl:template match="*" /> <xsl:template match="/"> <foo><xsl:value-of select="$bar" /></foo> </xsl:template> </xsl:stylesheet>''') st = etree.XSLT(style) res = st.apply(tree, bar="'Bar'") self.assertEquals('''\ <?xml version="1.0"?> <foo>Bar</foo> ''', st.tostring(res)) # apply without needed parameter will lead to XSLTApplyError self.assertRaises(etree.XSLTApplyError, st.apply, tree) ---------------------------------------------------------------------- Apparently, the comment is wrong here... I'll have to find some time to look into this, unless someone has an idea? Could anyone test this under Windows and maybe figure out what happens here? Sometimes this means that an unexpected exception is raised instead - or none at all? Stefan

On Fri, Oct 27, 2006 at 10:55:11PM +0200, Stefan Behnel wrote: | Thanks. Most of those are the usual Windows bugs that you can't delete an open | file. I fixed some and just silenced the remaining two - shouldn't be too much | of a problem if tiny temporary files are not deleted after running the test | cases (which is a rare enough event anyway...) Yeah, as long as they are tiny :) I will be running the tests constantly on that box. Maybe I can setup some task to clean up $TMP. | There's one problem left, though, and I don't have any idea where it might | come from. | | ====================================================================== | FAIL: test_xslt_parameters (lxml.tests.test_xslt.ETreeXSLTTestCase) | ---------------------------------------------------------------------- | Traceback (most recent call last): | File "C:\pybots\slave\trunk.dasilva-x86\build\lib\unittest.py", line 260, in run | testMethod() | File "C:\pybots\slave\python-tool\lxml-trunk\src\lxml\tests\test_xslt.py", | line 210, in test_xslt_parameters | st.apply, tree) | File "C:\pybots\slave\trunk.dasilva-x86\build\lib\unittest.py", line 326, in | failUnlessRaises | raise self.failureException, "%s not raised" % excName | AssertionError: XSLTApplyError not raised | ---------------------------------------------------------------------- | | Test case being: | | ---------------------------------------------------------------------- | def test_xslt_parameters(self): | tree = self.parse('<a><b>B</b><c>C</c></a>') | style = self.parse('''\ | <xsl:stylesheet version="1.0" | xmlns:xsl="http://www.w3.org/1999/XSL/Transform"> | <xsl:template match="*" /> | <xsl:template match="/"> | <foo><xsl:value-of select="$bar" /></foo> | </xsl:template> | </xsl:stylesheet>''') | | st = etree.XSLT(style) | res = st.apply(tree, bar="'Bar'") | self.assertEquals('''\ | <?xml version="1.0"?> | <foo>Bar</foo> | ''', | st.tostring(res)) | # apply without needed parameter will lead to XSLTApplyError | self.assertRaises(etree.XSLTApplyError, | st.apply, tree) | ---------------------------------------------------------------------- | | Apparently, the comment is wrong here... | | I'll have to find some time to look into this, unless someone has an idea? | Could anyone test this under Windows and maybe figure out what happens here? | Sometimes this means that an unexpected exception is raised instead - or none | at all? I can look at that later today. -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

| Apparently, the comment is wrong here... | | I'll have to find some time to look into this, unless someone has an idea? | Could anyone test this under Windows and maybe figure out what happens here? | Sometimes this means that an unexpected exception is raised instead - or none | at all? So here's what that line gives: (Pdb) p st.tostring(st.apply(tree)) '<?xml version="1.0"?>\n<foo/>\n' Looks like it just assumed the parameter was empty or something. -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi Stefan, On Fri, Oct 27, 2006 at 09:06:59PM -0300, Sidnei da Silva wrote: | | Apparently, the comment is wrong here... | | | | I'll have to find some time to look into this, unless someone has an idea? | | Could anyone test this under Windows and maybe figure out what happens here? | | Sometimes this means that an unexpected exception is raised instead - or none | | at all? | | So here's what that line gives: | | (Pdb) p st.tostring(st.apply(tree)) | '<?xml version="1.0"?>\n<foo/>\n' | | Looks like it just assumed the parameter was empty or something. Did you had a chance to look at the XSLTApplyError not being raised? Does that test fail on Linux? Maybe it's an issue with the version of libxml2 that I'm using on the buildbot? -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi Sidnei, Sidnei da Silva wrote:
Yes, I looked into it and decided it's not critical enough to delay 1.1.2 even more.
Does that test fail on Linux?
No, not on my machine.
Maybe it's an issue with the version of libxml2 that I'm using on the buildbot?
I tried with libxml2 2.6.24 and 2.6.26. Both pass the test nicely. I also looked through the code path and couldn't find anything obvious that would behave differently on different systems. I have no idea why that test fails on the buildbot. BTW, the buildbot logs (all of them) seem to be broken currently, don't know where that comes from. Stefan

On Mon, Oct 30, 2006 at 06:28:56PM +0100, Stefan Behnel wrote: | I tried with libxml2 2.6.24 and 2.6.26. Both pass the test nicely. I also | looked through the code path and couldn't find anything obvious that would | behave differently on different systems. I have no idea why that test fails on | the buildbot. I see that you added some extra output with the version numbers to the output. http://tinyurl.com/ymbmrt | BTW, the buildbot logs (all of them) seem to be broken currently, don't know | where that comes from. Yeah, the master got out of sync, somehow. It's working again now. -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

On Fri, Oct 27, 2006 at 10:55:11PM +0200, Stefan Behnel wrote: | Thanks. Most of those are the usual Windows bugs that you can't delete an open | file. I fixed some and just silenced the remaining two - shouldn't be too much | of a problem if tiny temporary files are not deleted after running the test | cases (which is a rare enough event anyway...) Two issues here: - In Python2.4 it raises OSError instead of WindowsError. I guess that is one of the changes in Python2.5. - I believe that this might be a real bug that needs fixing. Why it might be a bug: - I looked at the source in lxml and I see that this ends up calling xmlparser.xmlCtxtReadFile, which just delegates down to libxml2. Well, somewhere in there it seems like the file is read but not closed. By trial-and-failure, I've come up with the attached patch, which fixes the failures on Windows. Someone more experienced should review this. careful-not-to-hide-the-dirt-under-the-rug'ly yours, -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi Sidnei, Sidnei da Silva wrote:
Good to know.
You got me convinced. I think that's because we are using the context reusing calls (xmlCtxt*). They require calling xmlCtxtReset afterwards to clean up both the input stack and memory resources. This is normally called automatically when using the parser context the next time (which is why there never were any enduring side effects), but waiting for that has the temporal side effect of leaving the input stream open when passing control back to the user code. Now, the problem is, running xmlCtxtReset can currently segfault in some cases, so we can't just call it carelessly. I played with it a bit to figure out in which cases it can be called, but it doesn't look like we can safely call it in every case where it would make sense. Guess I'll file a bug report on it and try to come up with a work-around... Stefan

On Sat, Oct 28, 2006 at 10:05:14AM +0200, Stefan Behnel wrote: | You got me convinced. I think that's because we are using the context reusing | calls (xmlCtxt*). They require calling xmlCtxtReset afterwards to clean up | both the input stack and memory resources. This is normally called | automatically when using the parser context the next time (which is why there | never were any enduring side effects), but waiting for that has the temporal | side effect of leaving the input stream open when passing control back to the | user code. | | Now, the problem is, running xmlCtxtReset can currently segfault in some | cases, so we can't just call it carelessly. I played with it a bit to figure | out in which cases it can be called, but it doesn't look like we can safely | call it in every case where it would make sense. Guess I'll file a bug report | on it and try to come up with a work-around... Erm.... did you look at the attached patch? It just frees ctxt->input if its not NULL. I guess you're looking for a generic fix though. -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi Sidnei, Sidnei da Silva wrote:
Not only generic. Pending open files is only a symptom here. The real problem is that none of the resources allocated for parsing is freed before you call the parser again (in which case new resources will be allocated right away). So popping the input streams fixes the windows problem, but calling xmlClearParserCtxt() after parsing would be the right thing to do - if it didn't crash. Stefan

On Sat, Oct 28, 2006 at 03:16:43PM +0200, Stefan Behnel wrote: | Not only generic. Pending open files is only a symptom here. The real problem | is that none of the resources allocated for parsing is freed before you call | the parser again (in which case new resources will be allocated right away). | | So popping the input streams fixes the windows problem, but calling | xmlClearParserCtxt() after parsing would be the right thing to do - if it | didn't crash. Thanks for the explanation. That makes a lot of sense. A question though. Is the parser context expensive to allocate? Why not use xmlFreeParserCtxt() and allocate a new one instead of xmlResetParserCtxt()? -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

On Sat, Oct 28, 2006 at 03:43:05PM +0200, Stefan Behnel wrote: | It is pretty expensive to allocate. There are some hash-table allocations | involved that are rather costly (that's why there is a function for resetting | the context). In lxml, we try to reuse context objects wherever possible. One last question, do you have a small test that can reproduce the segfault or is it just random? I would like to spend some time tracking that one down. -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi, Sidnei da Silva wrote:
No need to do that. It's in line 12837 in file parser.c. Apparently, the assumption that ctxt->spaceTab has always been initialised when calling xmlCtxtReset() is wrong. Here's my bug report: http://bugzilla.gnome.org/show_bug.cgi?id=366161 My current work around is to do the NULL check myself and to initialise it the way libxml2 normally does before calling the reset. That's what's I'd normally expect libxml2 to do... BTW, I don't know in which cases this field remains uninitialised, so if you want to add some more infos to the bug report, feel free to investigate. I only know that it definitely happens in the case where we call xmlCtxtReadFile(). It is possible that this requires previous runs of the parser, maybe even a failed run preceding the crash, can't tell... Stefan

On Sat, Oct 28, 2006 at 04:12:30PM +0200, Stefan Behnel wrote: | No need to do that. It's in line 12837 in file parser.c. Apparently, the | assumption that ctxt->spaceTab has always been initialised when calling | xmlCtxtReset() is wrong. Here's my bug report: | | http://bugzilla.gnome.org/show_bug.cgi?id=366161 | | My current work around is to do the NULL check myself and to initialise it the | way libxml2 normally does before calling the reset. That's what's I'd normally | expect libxml2 to do... Yes, that check is certainly missing. Note that htmlCtxtReset() does the check! I added that info to the bug report. | BTW, I don't know in which cases this field remains uninitialised, so if you | want to add some more infos to the bug report, feel free to investigate. I | only know that it definitely happens in the case where we call | xmlCtxtReadFile(). It is possible that this requires previous runs of the | parser, maybe even a failed run preceding the crash, can't tell... Maybe in spacePush, if memory allocation fails. That seems a bit odd though... unless you're really short on memory :) -- Sidnei da Silva Enfold Systems http://enfoldsystems.com Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi Sidnei, Sidnei da Silva wrote:
I think I was on the wrong track. It's a bug in libxml2, but not the right one. The crash appears somewhere in the long doctest in api.txt, most likely in a place where errors are tested when parsing from a string. That's not what we are looking for. You said that your patch fixes the problem, are you certain about that? Because calling xmlCtxtReset() should do exactly that (and a lot more) and it doesn't seem to solve the problem - according to the buildbot. Stefan

On Sat, Oct 28, 2006 at 09:55:55PM +0200, Stefan Behnel wrote: | I think I was on the wrong track. It's a bug in libxml2, but not the right | one. The crash appears somewhere in the long doctest in api.txt, most likely | in a place where errors are tested when parsing from a string. That's not what | we are looking for. Ok... but that's no excuse for the check in HTMLparser to not be done on parser. | You said that your patch fixes the problem, are you certain about that? | Because calling xmlCtxtReset() should do exactly that (and a lot more) and it | doesn't seem to solve the problem - according to the buildbot. Yes, it does solve the problem for me on Python 2.4, ie, OSError is not raised after applying my patch. -- -e Sidnei da Silva -e Enfold Systems http://enfoldsystems.com -e Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

| | You said that your patch fixes the problem, are you certain about that? | | Because calling xmlCtxtReset() should do exactly that (and a lot more) and it | | doesn't seem to solve the problem - according to the buildbot. I see that you only call xmlClearContext if spaceTab is not NULL. Maybe that's the issue. -- -e Sidnei da Silva -e Enfold Systems http://enfoldsystems.com -e Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214

Hi Sidnei, Sidnei da Silva wrote:
No, I also tried initialising spaceTab by hand to always call reset(). No difference. So this is definitely not the problem. And it really makes me wonder how your patch can work if xmlClearParserCtxt() does not, because your code snippet is straight in there and I can't see a way it should not get executed if clear() is called. AFAICT, with the call to clear(), all input streams and memory resources should get freed after parsing, so that's all I wanted. And still the problem of a file descriptor staying open remains. I'll wait for the next buildbot run to see, but if that fails, I'll really get clueless... Note, BTW, that the error occurs in a finally block, so maybe it's already shadowing an exception? Stefan

On Sun, Oct 29, 2006 at 12:05:40AM +0200, Stefan Behnel wrote: | No, I also tried initialising spaceTab by hand to always call reset(). No | difference. So this is definitely not the problem. And it really makes me | wonder how your patch can work if xmlClearParserCtxt() does not, because your | code snippet is straight in there and I can't see a way it should not get | executed if clear() is called. | | AFAICT, with the call to clear(), all input streams and memory resources | should get freed after parsing, so that's all I wanted. And still the problem | of a file descriptor staying open remains. I'll wait for the next buildbot run | to see, but if that fails, I'll really get clueless... FWIW, I reverted my patch and did a svn up, and your changes seem to work here. No segfault or anything. The buildbot seems to be in some funny state now, maybe due to some checkin on Python 2.5/trunk. -- -e Sidnei da Silva -e Enfold Systems http://enfoldsystems.com -e Fax +1 832 201 8856 Office +1 713 942 2377 Ext 214
participants (3)
-
Sidnei da Silva
-
Sidnei da Silva
-
Stefan Behnel