Skip to content

Conversation

@dvdplm
Copy link
Contributor

@dvdplm dvdplm commented May 4, 2021

Add batch request support for the websocket server. Continuation of #292.

Closes #277

TODO:

  • better tests
  • sort out error, switch from InvalidParams to CallError::Failed
  • benchmarks

Co-authored-by: Niklas Adolfsson <[email protected]>
Ok(res) => send_response(id, tx, res),
Err(InvalidParams) => send_error(id, tx, JsonRpcErrorCode::InvalidParams.into()),
// TODO: this looks wonky...
Err(CallError::InvalidParams(InvalidParams)) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this InvalidParams type and just use to InvalidParams variant,

perhaps link to #299

Copy link
Contributor

Choose a reason for hiding this comment

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

thus, if CallError would implement a trait or something JsonRpcErrorCode then you could just to err.to_error_code() or something similar

Copy link
Contributor

@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.

Looks overall good, not exactly sure of the overhead w.r.t spawning an unbounded channel per batch request.

Should be faster than reusing the existing channel and we "will" limit incoming requests based on payload size anyway so should be fine w.r.t OOM/DOS related issues.

@niklasad1
Copy link
Contributor

An additional bonus to this PR, would be to add integration tests for batch requests (both HTTP and WS)

Could address it in another PR also.

Return app-level error when call fails
@dvdplm dvdplm marked this pull request as ready for review May 6, 2021 10:34
@dvdplm dvdplm requested a review from insipx May 6, 2021 12:15
@dvdplm dvdplm self-assigned this May 6, 2021
Copy link
Contributor

@insipx insipx 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 other than small nits 👍

dvdplm and others added 2 commits May 7, 2021 12:38
@dvdplm dvdplm merged commit 4038e9e into master May 7, 2021
@dvdplm dvdplm deleted the dp-batch-support-ws branch May 7, 2021 10:44
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.

[servers]: batch request support

4 participants