Fix quadratic-time backtracking when a reference link has no URL - #1631
Fix quadratic-time backtracking when a reference link has no URL#1631afonsojanu wants to merge 2 commits into
Conversation
A malformed reference definition line, one with a label but no URL
(just trailing whitespace after the colon), makes ReferenceProcessor's
regex backtrack badly. The pattern had two adjacent [ ]* groups around
an optional newline, both matching the same run of spaces, so for a
string of n spaces there were n+1 ways to split them before the engine
gave up and tried the next split. That turns markdown.markdown('[id]:'
+ ' ' * 50000) into an eight second call instead of a near-instant one,
and it gets worse fast as the input grows.
Rewrote the pattern so the leading run of spaces is consumed greedily
by a single group, with the optional newline plus more spaces folded
into one non-ambiguous alternative after it. Verified this produces
identical matches (and identical groups) as the old pattern on the
handful of valid reference-link shapes the tests already cover, and
added a dedicated regression test that fails on unmodified master and
passes with the fix.
Fixes Python-Markdown#798.
| See https://github.com/Python-Markdown/markdown/issues/798 | ||
| """ | ||
| text = '[id]:' + (' ' * 50000) | ||
| start = time.time() |
There was a problem hiding this comment.
Timed tests should likely be avoided as some systems may be running some slow hardware.
facelessuser
left a comment
facelessuser
left a comment
There was a problem hiding this comment.
Outside the timing tests, I think this looks okay. Please remove the timed test.
|
@afonsojanu you have deleted the PR template and have not provided an AI disclosure statement. I will not review or merge your PR until one is provided. Also, as @facelessuser noted, it is not appropriate to time tests. They can be run a very slow systems and will fail. This will be closed if a fix is not provided. |
waylan
added
needs-review
facelessuser pointed out that timing a test is fragile on slower hardware, so the regression test now only checks that the malformed reference is rendered correctly, without asserting anything about how long it takes. The regex fix itself is what prevents the quadratic blowup; ran the case manually and it completes in well under a millisecond now, versus roughly 8 seconds before the fix.
|
Thanks for the heads up, @waylan — sorry about that, I should have kept the template. Just updated the PR description with the AI disclosure checkbox filled in and the rest of the checklist, and pushed a commit that drops the timed assertion @facelessuser flagged. The test now only checks that the malformed reference still renders correctly (I confirmed by hand it runs in under a millisecond with the fix, versus ~8s before, no need to assert on wall-clock time in CI). |
3 participants
Fixes #798 (specifically the sub-case flagged by @stsewd in this comment, a regex in
ReferenceProcessor).markdown.markdown('[id]:' + ' ' * 50000)currently takes about 8 seconds. The reference-link regex has two adjacent[ ]*groups around an optional newline ([ ]*\n?[ ]*) sitting right before a mandatory non-whitespace group. When there's no newline, those two groups both match the same run of spaces, so for n trailing spaces there are n+1 ways to split them between the two groups before the engine gives up looking for the URL. That's quadratic in the length of the run.I rewrote it so the first run of spaces is consumed by a single greedy group, with the newline-plus-more-spaces case folded into one unambiguous alternative right after (
[ ]*(?:\n[ ]*)?). Checked this against the reference-link shapes the existing test suite covers (with/without title, title on its own line, id/url split across a line break, leading indent) and got byte-for-byte identical match groups on all of them.Added a new test file (
test_reference_links.py) covering a few basic reference-link cases plus the regression case from #798. Per the review feedback, the regression test no longer times anything, it just checks the malformed reference still renders correctly; I confirmed by hand that it runs in well under a millisecond with the fix versus ~8 seconds on unmodifiedmaster. Full existing suite (1096 tests) still passes, and flake8 is clean on the touched files.No CLA or DCO step in this repo as far as I could find.
AI Assistance Disclosure
I used Claude (Sonnet 5) to help investigate and write this fix, and reviewed and verified the diff and the test results myself before submitting.
Checklist