Skip to content

lm_sensors: update broken json parser #2289

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lasers
Copy link
Contributor

@lasers lasers commented Jun 26, 2025

This changes how we parse broken json blocks from upstream. Instead of splitting chunks based on }\n{ , this just run every line until we get a valid json chunk before we add it to a dictionary.

Ought to address #2226 (comment). /cc @MacGyverNL for testing.

@MacGyverNL
Copy link

Gonna be a few days before I can test this, I'm afraid. The system on which this is a problem is currently undergoing maintenance.

@MacGyverNL
Copy link

Can confirm this works, although with this splitting, my case never hits the codepath with JSONDecodeErrors. So I can't testify to the effectiveness / robustness of building the chunk across multiple lines.

@lasers
Copy link
Contributor Author

lasers commented Jul 22, 2025

@MacGyverNL Can you test this again? The first commit was simple, but will fail on several lines (repeatedly) until we get a valid JSON (again, repeatedly). I took second look at this and wondered if we can make do with this (second commit) instead.

@lasers lasers force-pushed the lm_sensors-new-chunk branch 6 times, most recently from e637990 to ca92fd4 Compare July 22, 2025 14:12
@MacGyverNL
Copy link

Your most recent version does not chunk, I think you mishandled the caret and dollar placement. I don't know what your goal was with those, but if I fix those naively (r'}$\n^{'), then it crashes again on the first chunk, because the split still gobbles up one closing bracket, and they end up unbalanced. Don't you want something like a positive lookbehind / lookahead, along the lines of r'(?<=})$\n^(?={)'? But I don't know how expensive lookbehind / lookahead matching is when compared to the older failure type.

@lasers lasers force-pushed the lm_sensors-new-chunk branch from ca92fd4 to ecfe23e Compare July 22, 2025 15:24
@lasers
Copy link
Contributor Author

lasers commented Jul 22, 2025

Noted! It's a better solution. Please try it again.

@lasers lasers force-pushed the lm_sensors-new-chunk branch 2 times, most recently from b22b0ae to 773b9ff Compare July 22, 2025 15:29
@MacGyverNL
Copy link

MacGyverNL commented Jul 22, 2025

Yeah this should work (edit for clarity: the commit I tested, 773b9ff, doesn't), once you get the ^ and $ right. I think what's confusing you is where to match start- and end-of-line, especially in multiline mode. These aren't indicators of where the pattern starts, they're match points you'd normally use to match only at the start and end of a line. The way I read the documentation, in multiline mode they still match right before and after each newline. So in principle, because we're explicitly matching the newline, you don't actually need them at all. self.pattern = re.compile(r'(?<=})\n(?={)', re.MULTILINE) suffices (and works, already tested it).

(I know I could commit this but I don't currently have a fork, I'm just manual-editing the python code of my system package, and it's a single line.)

@lasers lasers force-pushed the lm_sensors-new-chunk branch from 773b9ff to 5a401fb Compare July 22, 2025 16:41
@lasers
Copy link
Contributor Author

lasers commented Jul 22, 2025

Thank you for your help / looking into this. We should be good here now.

@lasers lasers force-pushed the lm_sensors-new-chunk branch from 5a401fb to 93b3b7e Compare July 22, 2025 16:42
@lasers lasers force-pushed the lm_sensors-new-chunk branch from 93b3b7e to a82cc6b Compare August 8, 2025 13:39
@lasers
Copy link
Contributor Author

lasers commented Aug 10, 2025

@ultrabug \o/

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