Skip to content

prevent panic on parser io error #29

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

Merged
merged 1 commit into from
Jul 12, 2025
Merged

prevent panic on parser io error #29

merged 1 commit into from
Jul 12, 2025

Conversation

kpwebb
Copy link
Contributor

@kpwebb kpwebb commented Jul 8, 2025

The library panics on files that don't contain valid text. I added an error check ahead of the unwrap on line, and a new Parser IO Error type.

@gwbres
Copy link
Contributor

gwbres commented Jul 8, 2025

Hello @kpwebb,

thank you for the suggestion 👍

I think I wrote this unwrap for the simple reason I don't understand what might go wrong here.
Is that an I/O (reading) issue, or invalid UTF-8 that could not be interpreted ?
Either way, I'm fine with catching the error.

But if this happens in the header, the parsing will most likely fail completely.

@kpwebb
Copy link
Contributor Author

kpwebb commented Jul 9, 2025

Yeah the error I'm seeing: when an invalid file is opened the lib panics due to UTF-8 encoding issues. I'm fine with a more generic error being used as a catchall as a panic causes a lot of problems for downstream users.

Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
@gwbres
Copy link
Contributor

gwbres commented Jul 12, 2025

I/O error is already caught & wrapped, we can simply replace the unwrap with ? that would return an Error at this point.
The debug/panic trace is then the std::io trace

@gwbres gwbres merged commit 2e3828e into nav-solutions:main Jul 12, 2025
1 check passed
@gwbres
Copy link
Contributor

gwbres commented Jul 12, 2025

If you don't mind, I want to merge my pending PR and close a few issues before publishing a new release.
This will probably take 1 or 2 days

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