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

Better handling of malformed messages #69

Merged
merged 3 commits into from
May 1, 2023
Merged

Better handling of malformed messages #69

merged 3 commits into from
May 1, 2023

Conversation

jwoertink
Copy link
Collaborator

Fixes #67

This PR refactors how the Payload is created in order to better handle when a message comes through with malformed data. It also gives us a bit of a performance boost by moving the Payload over to structs!

❯ ./bench 
new_payload 677.95  (  1.48ms) (± 5.55%)  2.92MB/op        fastest
old_payload 513.67  (  1.95ms) (± 5.67%)  4.37MB/op   1.32× slower

Explanation

The current Payload makes a lot of assumptions about the shape of the JSON being parsed. It first assumes valid JSON is passed in, then assumes that the valid JSON is an Object. The handler generally just does a blanket JSON::ParsingError catch on this.

Now if we get an empty string, we can just ignore it, and if the JSON structure is valid, it'll be a full serialized object we can pass around easier. If the Payload can't be deserialized, then we raise the serialized error and let the handler do as it does.

container: crystallang/crystal:${{ matrix.crystal_version }}-alpine
container: crystallang/crystal:${{ matrix.crystal_version }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is because Ameba tanks on 1.6.0 under alpine

ref: crystal-ameba/ameba#354

src/cable/payload.cr Outdated Show resolved Hide resolved
end
end

it "ignores incorrect json structures" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice! so do not consider what I commented on the issue lol

@@ -1,82 +1,118 @@
module Cable
class Payload
Copy link
Collaborator

Choose a reason for hiding this comment

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

ohhh now I see what you're saying about the payload on the connection, maybe I misunderstood the issue 🤦

this is cool for sure, even better with the performance boost 👏

…stead of generating a new one to ensure the result is always the same
@jwoertink jwoertink merged commit b05e0f4 into master May 1, 2023
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.

Accounting for bad data
2 participants