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

Do not parse packets as utf8 #151

Closed
iffyio opened this issue Dec 3, 2020 · 5 comments · Fixed by #156
Closed

Do not parse packets as utf8 #151

iffyio opened this issue Dec 3, 2020 · 5 comments · Fixed by #156
Labels
good first issue Good for newcomers help wanted Extra attention is needed kind/cleanup Refactoring code, fixing up documentation, etc

Comments

@iffyio
Copy link
Collaborator

iffyio commented Dec 3, 2020

In some parts of session.rs and server/mod.rs we currently try to parse packets as utf8 and will fail to do so and panic with raw bytes. We likely need to remove these lines

@markmandel
Copy link
Member

Agreed, they should probably be something more like:

"contents" => from_utf8(packet).unwrap.or_else(|| format!("<raw bytes. len: {}>", packet.len())) - or something like that

@markmandel markmandel added good first issue Good for newcomers help wanted Extra attention is needed kind/cleanup Refactoring code, fixing up documentation, etc labels Dec 4, 2020
@markmandel
Copy link
Member

You are going to laugh. I literally JUST hit this issue writing the compression filter.

Looks like I'm implementing a fix!

@iffyio
Copy link
Collaborator Author

iffyio commented Dec 5, 2020

I'm thinking it makes more sense to remove the log lines rather than work around it? Currently it seems to be unneeded work to try to parse every packet and create a log record each time only to discard it, especially since metrics tracks that info already.

@markmandel
Copy link
Member

I honestly find it really useful when debugging issues found by integration tests, because I can see where in the flow packets are failing.

markmandel added a commit that referenced this issue Dec 6, 2020
The current debug logging assumes a packet is utf-8, and if not, it
panics, which is bad.

This provides a fallback output if the packet is not utf-8, and
introduces a `debug` module to add tools to for debug builds and
debugging in general.

Closes #151
@iffyio
Copy link
Collaborator Author

iffyio commented Dec 7, 2020

I think this relates to #153 outcome in that since the log messages are only needed during development we can compile them entirely away? I'm thinking then we can log them at level trace so that they are explicitly enabled, temporarily when needed

markmandel added a commit that referenced this issue Dec 8, 2020
The current debug logging assumes a packet is utf-8, and if not, it
panics, which is bad.

This provides a fallback output if the packet is not utf-8, and
introduces a `debug` module to add tools to for debug builds and
debugging in general.

Closes #151
markmandel added a commit that referenced this issue Dec 10, 2020
* Don't panic on debug when packets aren't utf-8

The current debug logging assumes a packet is utf-8, and if not, it
panics, which is bad.

This provides a fallback output if the packet is not utf-8, and
introduces a `debug` module to add tools to for debug builds and
debugging in general.

Closes #151

* Review updates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed kind/cleanup Refactoring code, fixing up documentation, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants