Skip to content

Replace opensubtitlescom _search recursion with a generator implementation - #1336

Merged
getzze merged 2 commits into
Diaoul:mainfrom
Johnnynator:less-recursion
Dec 29, 2025
Merged

getzze merged 2 commits into
Diaoul:mainfrom
Johnnynator:less-recursion

Conversation

@Johnnynator

Copy link
Copy Markdown
Contributor

It can happen that a opensubtitlescom query might return hundreds of
pages, this can lead to a stackoverflow (especially on systems with a
smaller default thread stack size like musl libc)

@github-actions

github-actions Bot commented Dec 27, 2025 •

Copy link
Copy Markdown
Contributor

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/subliminal/providers
  opensubtitlescom.py
Project Total  

This report was generated by python-coverage-comment-action

@getzze getzze left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!
I added some suggestions to simplify the code.

Comment thread src/subliminal/providers/opensubtitlescom.py Outdated
Comment thread src/subliminal/providers/opensubtitlescom.py Outdated
Comment thread src/subliminal/providers/opensubtitlescom.py Outdated
@getzze

getzze commented Dec 28, 2025

Copy link
Copy Markdown
Collaborator

To mitigate this bug, since version 2.5.0 you can limit the number of pages: #1332

@Johnnynator

Copy link
Copy Markdown
Contributor Author

To mitigate this bug, since version 2.5.0 you can limit the number of pages: #1332

Thx for the info, I was aware of it and thought about just setting a default page limit. But not doing (deep) recursion in a language that doesn't do tail call elimination sounded like a better way :)

I replaced the usage of allow_more_pages, total_pages_reached, and abort with a break statement

@Johnnynator
Johnnynator force-pushed the less-recursion branch 6 times, most recently from 78fde4e to 6fa86ec Compare December 28, 2025 11:56
Comment thread src/subliminal/providers/opensubtitlescom.py Outdated
@getzze

getzze commented Dec 28, 2025

Copy link
Copy Markdown
Collaborator

Can you add a changelog entry, please?
You can use the following command (I let you put the description) and then commit the created file:

towncrier create -c "DESCRIPTION" 1336.provider.rst

…ation

It can happen that a opensubtitlescom query might return hundreds of
pages, this can lead to a stackoverflow (especially on systems with a
smaller default thread stack size like musl libc)
@Johnnynator

Copy link
Copy Markdown
Contributor Author

Added changelog.d/1336.provider.rst

Comment thread changelog.d/1336.provider.rst Outdated
@getzze
getzze merged commit 8bb5843 into Diaoul:main Dec 29, 2025
29 checks passed
@getzze

getzze commented Dec 29, 2025

Copy link
Copy Markdown
Collaborator

Thank you for your contribution!

h3llrais3r pushed a commit to h3llrais3r/subliminal that referenced this pull request Dec 30, 2025
…ation (Diaoul#1336)

* Replace opensubtitlescom _search recursion with a generator implementation

It can happen that a opensubtitlescom query might return hundreds of
pages, this can lead to a stackoverflow (especially on systems with a
smaller default thread stack size like musl libc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants