classification
Title: test_faulthandler can fail if install path is too long
Type: Stage:
Components: Tests Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: chris.jerdonek, haypo, ned.deily, python-dev
Priority: low Keywords: patch

Created on 2012-07-27 01:23 by ned.deily, last changed 2012-08-01 18:35 by chris.jerdonek. This issue is now closed.

Files
File name Uploaded Description Edit
issue-15463-1.patch chris.jerdonek, 2012-07-28 19:46 review
issue-15463-2.patch chris.jerdonek, 2012-07-30 13:48 review
Messages (23)
msg166531 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-27 01:23
======================================================================
FAIL: test_dump_traceback_threads (test.test_faulthandler.FaultHandlerTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/3.3/lib/python3.3/test/test_faulthandler.py", line 365, in test_dump_traceback_threads
    self.check_dump_traceback_threads(None)
  File "/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/3.3/lib/python3.3/test/test_faulthandler.py", line 361, in check_dump_traceback_threads
    self.assertRegex(output, regex)
AssertionError: Regex didn't match: '^Thread 0x[0-9a-f]+:\n(?:  File ".*threading.py", line [0-9]+ in [_a-z]+\n){1,3}  File "<string>", line 23 in run\n  File ".*threading.py", line [0-9]+ in _bootstrap_inner\n  File ".*threading.py", line [0-9]+ in _bootstrap\n\nCurrent thread XXX:\n  File "<string>", line 10 in dump\n  File "<string>", line 28 in <module>$' not found in 'Thread 0x0000000103a4a000:\n  File "/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/...", line 184 in wait\n  File "/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/...", line 330 in wait\n  File "<string>", line 23 in run\n  File "/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/...", line 639 in _bootstrap_inner\n  File "/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/...", line 616 in _bootstrap\n\nCurrent thread XXX:\n  File "<string>", line 10 in dump\n  File "<string>", line 28 in <module>'

======================================================================
FAIL: test_dump_traceback_threads_file (test.test_faulthandler.FaultHandlerTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/3.3/lib/python3.3/test/test_faulthandler.py", line 369, in test_dump_traceback_threads_file
    self.check_dump_traceback_threads(filename)
  File "/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/3.3/lib/python3.3/test/test_faulthandler.py", line 361, in check_dump_traceback_threads
    self.assertRegex(output, regex)
AssertionError: Regex didn't match: '^Thread 0x[0-9a-f]+:\n(?:  File ".*threading.py", line [0-9]+ in [_a-z]+\n){1,3}  File "<string>", line 23 in run\n  File ".*threading.py", line [0-9]+ in _bootstrap_inner\n  File ".*threading.py", line [0-9]+ in _bootstrap\n\nCurrent thread XXX:\n  File "<string>", line 8 in dump\n  File "<string>", line 28 in <module>$' not found in 'Thread 0x0000000105215000:\n  File "/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/...", line 184 in wait\n  File "/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/...", line 330 in wait\n  File "<string>", line 23 in run\n  File "/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/...", line 639 in _bootstrap_inner\n  File "/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/...", line 616 in _bootstrap\n\nCurrent thread XXX:\n  File "<string>", line 8 in dump\n  File "<string>", line 28 in <module>'

----------------------------------------------------------------------
Ran 27 tests in 20.582s

FAILED (failures=2)
test test_faulthandler failed

The two failing tests call check_dump_traceback_threads which has a regex to find threading.py in the traceback but the path is truncated in the traceback so "threading.py" doesn't appear.
msg166535 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-27 02:34
This might be obvious to some people, but for background purposes, this seems to be where the file name gets truncated (in the dump_frame() function):

    write(fd, "\"", 1);
    dump_ascii(fd, code->co_filename);
    write(fd, "\"", 1);

http://hg.python.org/cpython/file/ddf15cd9be4a/Python/traceback.c#l564

Out of curiosity, I wonder if we have a test to check the formatting of tracebacks for long file names, which would be nice to have.  I didn't see one.
msg166536 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-27 02:47
Without investigating further, my instinct would be to prefer to truncate in the middle, if possible, otherwise truncate the head of the path, rather than truncate the tail.
msg166548 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-07-27 07:51
faulthandler has arbitrary limits to avoid an unlimited loop if some
data are corrupted.

#define MAX_STRING_LENGTH 100
#define MAX_FRAME_DEPTH 100
#define MAX_NTHREADS 100

The string limit is maybe too short. MAX_STRING_LENGTH can be set to
500 characters, what do you think?

We may also add a function to change these limits at runtime.

> Without investigating further, my instinct would be to prefer to truncate in the middle,
> if possible, otherwise truncate the head of the path, rather than truncate the tail.

faulthandler does not trust anything, including the length of the
string. Change the limit to 500 characters should be safer.

Remember that faulthandler is called when something really bad
happens, like a segfault or an illegal instruction.

faulthandler may be less strict when it is called explicitly, ex:
faulthandler.dump_traceback().
msg166635 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-28 07:36
> We may also add a function to change these limits at runtime.

This sounds like a good idea.  Is there already an issue for this?  If not, I could create one.

Regardless of whether MAX_STRING_LENGTH is increased, my instinct is that the test should work for long install paths independent of the setting's value.  I wonder if there is a way to adjust the test so that the file name can still be checked in such a scenario (i.e. so that we do not simply add "or '...'" logic to the regular expression).
msg166650 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-07-28 12:35
>> We may also add a function to change these limits at runtime.
>
> This sounds like a good idea.  Is there already an issue for this?  If not, I could create one.

There is no such issue. You can create it: add me to the nosy list.
msg166651 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-07-28 12:39
> Regardless of whether MAX_STRING_LENGTH is increased, my instinct is that the test should work for long install paths independent of the setting's value.  I wonder if there is a way to adjust the test so that the file name can still be checked in such a scenario (i.e. so that we do not simply add "or '...'" logic to the regular expression).

The purpose of the test is to check that the traceback is correct:
it's important to check that the filename ends with "threading.py". If
you don't know in which file the bug occured, the traceback is
useless.

Changing the MAX_STRING_LENGTH should be enough to fix this issue
before Python 3.3 final.
msg166662 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-28 16:15
> There is no such issue. You can create it: add me to the nosy list.

I created issue 15479 to allow the limits to be changed at runtime.
msg166663 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-28 16:28
> The purpose of the test is to check that the traceback is correct:
> it's important to check that the filename ends with "threading.py".

I agree with you.  That is why I argued against including "or '...'" logic in the current test.

> Changing the MAX_STRING_LENGTH should be enough to fix this issue
> before Python 3.3 final.

That is fine.  However, I should also point out that the current test is not sufficient to test on all systems/installs that this issue has been fixed.  It would be good to add such a test (i.e. one that can, for example, simulate a long install path).
msg166681 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-28 19:46
Here is a patch to the tests that allows the tests to pass when the install path is long while still checking that file names show up correctly in the traceback.

One advantage of this patch's approach is that it provides a way to test the rendering of long paths on all systems/installs.  Simply create a test case by setting the scriptname to, for example--

scriptname = 'test_long_file_path_' + 200 * 'x'

This patch is just an illustration for discussion purposes and is not meant as a final patch.

Also, this patch does not fix the issue of the file names of long paths not getting rendered (which can be addressed by setting the limit to 500 as we discussed).  However, as I have noted this patch provides a way to test such a fix.
msg166876 - (view) Author: Roundup Robot (python-dev) Date: 2012-07-30 11:08
New changeset ff7fc6a91212 by Victor Stinner in branch 'default':
Issue #15463: the faulthandler module truncates strings to 500 characters,
http://hg.python.org/cpython/rev/ff7fc6a91212
msg166878 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-07-30 11:19
> Issue #15463: the faulthandler module truncates strings to 500 characters,

It should be able for most setup. See the issue #15479 if you consider that something better should be implemented.

Long path names are handled differently on each platform, I don't want to add a test checking a path longer than 500 characters. You might add a new test for truncated path if limits become configurable (#15479).

I consider this issue as resolved.
msg166881 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-30 11:25
Shouldn't a test be added to check that the issue has been fixed though (i.e. a test with a path longer than the original 100 characters)?

I provided a patch to show how this can easily be done.

I would be happy to write the test.
msg166883 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-07-30 11:46
> I provided a patch to show how this can easily be done.

Your patch does not create a path longer than 100 characters. I didn't want to write such test, but I can review a patch adding such test.

The limit is not only applied on the filename, but also on the function name. It is maybe easier (more portable) to declare a function with a very long name:

exec(compile("def " + "f"*600 + "(): raise ValueError()\n" + "f"*600 + "()", "a", "exec"))
msg166884 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-30 11:50
BTW, the applied patch does fix the problem originally reported. Thanks.
msg166885 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-30 11:51
> I didn't want to write such test, but I can review a patch adding such test.

Thank you.  I will provide a patch for your review.

I think a test for the path may be better as that was the original issue experienced.
msg166899 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-30 13:48
Attaching patch with test case for long script path.

This test failed on my system before the fix and passes after.
msg166902 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-30 14:43
Is it okay for me to reopen this issue to review the patch to add a test case for file paths longer than 100 characters, or should I create a new issue for that?
msg167152 - (view) Author: Roundup Robot (python-dev) Date: 2012-08-01 17:49
New changeset 6e03f9b72c61 by Victor Stinner in branch 'default':
Issue #15463: Write a test for faulthandler truncating the name of functions
http://hg.python.org/cpython/rev/6e03f9b72c61
msg167153 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-08-01 17:50
I added a test checking that faulthandler truncates strings to MAX_STRING_LENGTH characters using a very long function name. I consider this issue as done.
msg167159 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-08-01 18:11
> I didn't want to write such test, but I can review a patch adding such test.

Victor, did you see and review my patch?  There is currently no test to check that a long file path (longer than 100 characters) will render, which was the original issue that was fixed.  The patch provides such a test.
msg167160 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-08-01 18:13
> Victor, did you see and review my patch? There is currently no test to check that a long file path (longer than 100 characters) will render, which was the original issue that was fixed.  The patch provides such a test.

Yes, I saw your patch, but I don't want to create very long filename.
Not all platforms support paths longer than 500 characters. It is
exactly the same code to display function name and file names. I
prefer to use a very long function name, it's simple and portable.

My patch is different than yours: it checks that the string is
truncated, not that string shorter than the limit are not truncated.
msg167162 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-08-01 18:35
> Yes, I saw your patch, but I don't want to create very long filename.
Not all platforms support paths longer than 500 characters.

I think you may have a slight misunderstanding.  My patch was not meant to test paths over 500 characters.  It tests a path *under* 500 characters (but slightly over 100 characters).  And it has the same style and portability characteristics as the existing ".*threading.py" tests.

> My patch is different than yours: it checks that the string is
truncated, not that string shorter than the limit are not truncated.

Yes, your patch is different in two ways. :) It tests function names instead of paths, and length > 500 instead of 100 < length < 500.  I think there is value in adding a file path test because code changes over time, so it may not always be using the same code path as function names.

I agree that it is okay to leave out a file path test for longer than 500 characters for the portability reasons you stated.
History
Date User Action Args
2012-08-01 18:35:56chris.jerdoneksetmessages: + msg167162
2012-08-01 18:13:43hayposetmessages: + msg167160
2012-08-01 18:11:15chris.jerdoneksetmessages: + msg167159
2012-08-01 17:50:56hayposetstatus: open -> closed
resolution: fixed
messages: + msg167153
2012-08-01 17:49:37python-devsetmessages: + msg167152
2012-07-30 15:50:08hayposetstatus: closed -> open
resolution: fixed -> (no value)
2012-07-30 14:43:43chris.jerdoneksetmessages: + msg166902
2012-07-30 13:48:12chris.jerdoneksetfiles: + issue-15463-2.patch

messages: + msg166899
2012-07-30 11:51:57chris.jerdoneksetmessages: + msg166885
2012-07-30 11:50:35ned.deilysetmessages: + msg166884
2012-07-30 11:46:03hayposetmessages: + msg166883
2012-07-30 11:25:31chris.jerdoneksetmessages: + msg166881
2012-07-30 11:19:55hayposetstatus: open -> closed
resolution: fixed
messages: + msg166878
2012-07-30 11:08:40python-devsetnosy: + python-dev
messages: + msg166876
2012-07-28 19:46:56chris.jerdoneksetfiles: + issue-15463-1.patch
keywords: + patch
messages: + msg166681
2012-07-28 16:28:41chris.jerdoneksetmessages: + msg166663
2012-07-28 16:15:28chris.jerdoneksetmessages: + msg166662
2012-07-28 12:39:05hayposetmessages: + msg166651
2012-07-28 12:35:20hayposetmessages: + msg166650
2012-07-28 07:36:19chris.jerdoneksetmessages: + msg166635
2012-07-27 07:51:55hayposetmessages: + msg166548
2012-07-27 02:47:52ned.deilysetmessages: + msg166536
2012-07-27 02:34:02chris.jerdoneksetnosy: + chris.jerdonek
messages: + msg166535
2012-07-27 01:23:07ned.deilycreate