Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Regex::PCRE2#finalize redefinition #13309

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Apr 12, 2023

This redefinition made very simple snippets like below run out of memory eventually:

while true
  Regex.new("")
end

It might be related to #13306 and some other recent memory blow-ups too.

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:text labels Apr 12, 2023
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Great find 🚀 🙈
But generating the docs still stalls: https://github.com/crystal-lang/crystal/actions/runs/4683438004/jobs/8298519048?pr=13309
So it doesn't seem to fix that.

@straight-shoota straight-shoota added this to the 1.8.0 milestone Apr 13, 2023
@HertzDevil
Copy link
Contributor Author

d60fe6c: LibPCRE2.code_free can be called freely even in Crystal's interpreter, because it is a regular lib fun, not an extern Proc-typed variable, unlike LibPCRE.free (#12495).

@beta-ziliani
Copy link
Member

The interpreter spec failure seems to be unrelated (it's been failing occasionally on the same point)

@straight-shoota straight-shoota merged commit 927f041 into crystal-lang:master Apr 13, 2023
@HertzDevil HertzDevil deleted the bug/regex-pcre2-finalize branch April 14, 2023 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants