Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[http server] Batch requests #292
[http server] Batch requests #292
Changes from 17 commits
f047879
ead0c4e
dd70b49
3b6e47d
3e7d3c9
d45be62
2db1028
a229deb
361e6cf
baf2d08
c106710
ee40258
8b6e213
ec84408
f5212a6
1fea05e
8debdfc
af873d0
78f3b25
e2cd294
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to make this a standalone function? the
start
fn is already fairly long, i think it would be good to move some code outThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, and I agree, and I tried but couldn't make it work; I'd have to look up the
method
from themethods
hash map instart()
and pass it along so it didn't really shorten up the code much.Lately I've started to shift my views on when it's right to refactor for briefness. The rule of thumb is still "terse is good", but for some cases I've started to think it's alright to keep the code long when it's "the main loop", whatever that means for each specific case, i.e. the most important thing that a given program does. Splitting things up too much can force the reader to jump around more than is ideal and even if it's long it is sometimes more readable to keep it all in one place. I know this is hand wavey (and for this case here I wanted to do exactly what you suggest), but yeah, just wanted to share those thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mainly because of the closure that is passed to hyper but yeah this changes made the code really quite hard to read, it wasn't great before either.
Maybe we split it the response handling to helper functions, basically you have to read the bytes here to take ownership over it which used to later to build a message to send to the background task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I found this more readable but it's still quite long ^^
(maybe because I wrote it lol)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K, I tried another approach, moving
execute
to be a method onServer
so I can accessself.root
(i.e. the function pointers we call to run the request), but it doesn't work becauseself
is moved into the closure when callingmake_service_fn()
.On the client side it's a bit easier because we don't have to call anything. Or I'm too limited to see how to do it! Pointers welcome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just add another parameter,
&Methods
(we'd have to import methods from jsonrpsee_util) to the execute function so execute becomes:and then call it like
execute(&methods, id, &tx, &method_name, params);
I guess the tradeoff is having a long function signature for
execute
, though.I think I agree with your analysis of
main_loops
though. Often it can be hard to make them shorter and would result in more confusing code than just keeping it all together. In this case I thinkexecute
fn is self-explanatory enough in that it just executes the right function for the rpc call.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@insipx I could have sworn I tried that; your version works too. I can go either way here, @niklasad1 thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW the performance of @insipx's version is identical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niklasad1 What does the connected client see in this case, i.e. when the method they're calling fails? Shouldn't we be sending back the error here in addition to logging it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that what you're doing in #295 perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, #295 should fix that but it doesn't log it FWIW
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is performed to mark to that we are done sending any further messages?!
Such that we the receiver will receive eventually
Ok(None)
when all messages has been sent?I think it deserves a comment because it was not straightforward to me at least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly, I read this in the docs:
I don't know if it's necessary to close the channel in this case but thought maybe it's a "best practice" to do so and if we ever decide to execute batch requests on separate tasks (and threads) with deadlines it's good to make sure the channel can't be written to. I'll add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I think so because otherwise you have to drop the sender to close the channel. Because we have control of over both the sender and receiver (the sender is just borrowed by the call closure)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably because
Id
is regarded aNone
not sure, so a typed Id could fix this