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

Sniff the first byte to glean if the incoming request is a single or batch request #419

Merged
merged 6 commits into from
Aug 13, 2021

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Jul 15, 2021

This works around the serde limitations around untagged enums and RawValue by instead looking at the first byte of the request: if it's a [ then it's a batch request; if it's a { then it's a single request.

The http case is less nice because there we have notifications (i.e. requests without an id, requiring no response).

Also see #420 as an alternative to this.

Closes #296

dvdplm added 2 commits July 15, 2021 17:42
…batch request

This works around the serde limitations around `untagged` enums and `RawValue`.
@dvdplm dvdplm requested a review from maciejhirsz July 15, 2021 15:48
Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

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

Looks good, just a question on one bit that might cause panic on runtime?

ws-server/src/server.rs Outdated Show resolved Hide resolved
@dvdplm dvdplm requested a review from niklasad1 August 12, 2021 11:01
Err(_) => (Id::Null, JsonRpcErrorCode::ParseError),
};
send_error(id, &tx, code.into());
// Bacth of requests or notifications
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Bacth of requests or notifications
// Batch of requests or notifications

/// Figure out if this is a sufficiently complete request that we can extract an [`Id`] out of, or just plain
/// unparseable garbage.
pub fn prepare_error(data: &[u8]) -> (Id<'_>, JsonRpcErrorCode) {
match serde_json::from_slice::<JsonRpcInvalidRequest>(&data) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
match serde_json::from_slice::<JsonRpcInvalidRequest>(&data) {
match serde_json::from_slice::<JsonRpcInvalidRequest>(data) {

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM,

Did you notice any performance gains compared to master?

@dvdplm dvdplm merged commit fba533f into master Aug 13, 2021
@dvdplm dvdplm deleted the dp-single-or-batch-sniff-first-byte branch August 13, 2021 07:52
@dvdplm
Copy link
Contributor Author

dvdplm commented Aug 13, 2021

Did you notice any performance gains compared to master?

I did not run the benchmarks tbh. I should have.

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.

#[serde(untagged)] doesn't work with RawValue
3 participants