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

fixes #36, switch to HTTP/1.0 and drop Connection: keep-alive from httpclient #39

Closed
wants to merge 5 commits into from
Closed

Conversation

jangko
Copy link
Contributor

@jangko jangko commented Nov 9, 2018

No description provided.

@zah zah requested a review from cheatfate November 12, 2018 20:55
Copy link
Contributor

@coffeepots coffeepots left a comment

Choose a reason for hiding this comment

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

This LGTM but @cheatfate can you have a look too. One thing that makes me pause is I was expecting chunked transfer to be placed in nim-http-utils as a more generic process, however perhaps it's better (aka less complex) to have it in place in RPC.

@coffeepots
Copy link
Contributor

Also I'm wondering how easy it would be to add some simple tests for the chunking. @jangko would this be easy for you to set up?

@jangko
Copy link
Contributor Author

jangko commented Nov 15, 2018

would this be easy for you to set up?

honestly, I have no idea where to start, that's why I left it blank.
Does it require me to modify the server too?
any clues perhaps?

@cheatfate
Copy link
Contributor

@jangko chunked transfer encoding is not supported by HTTP/1.0, and because geth properly supports HTTP specification you can use HTTP/1.0 requests and avoid chunked answers.

nim-json-rpc is also not the right place for chunked decoder.

@jangko
Copy link
Contributor Author

jangko commented Nov 16, 2018

luckily, parity also support HTTP/1.0.
does it mean we also drop Connection: keep-alive field too when connecting with HTTP/1.0?
is that acceptable?

@cheatfate
Copy link
Contributor

Connection: keep-alive is also from HTTP/1.1.

@jangko
Copy link
Contributor Author

jangko commented Nov 19, 2018

switching to HTTP/1.0 and removing Connection: keep-alive prove to be more problematic.
I think I will go through the hard way by putting it into httputils and implement chunked transfer decoding

@cheatfate
Copy link
Contributor

More then 100k requests per second is not enough for you @jangko, you need more speed?
I do not like your approach on chunked processing inside of nim-http-utils, because it looks not as solid improvement, but like temporary fix, it will not allow to easily extend functionality by adding gzip, deflate encoding.

@jangko
Copy link
Contributor Author

jangko commented Nov 20, 2018

I didn't say about speed, I've located the problem inside asyncdispatch2, perhaps we can pending that chunked transfer problem later and for now we can switch to HTTP/1.0 and drop keep-alive connection.

@jangko jangko changed the title fixes #36, add chunked tranfer encoding support to httpclient fixes #36, switch to HTTP/1.0 and drop Connection: keep-alive from httpclient Nov 20, 2018
@coffeepots
Copy link
Contributor

@jangko We've been discussing the chunking work and have come to the conclusion that we should stick with HTTP 1.0 for now (which you've already done here) and later have fully genericised chunking code further up in the tool chain.

@jangko
Copy link
Contributor Author

jangko commented Nov 29, 2018

done

@jangko
Copy link
Contributor Author

jangko commented Dec 26, 2018

closed in favor of 6a0b0ff

@jangko jangko closed this Dec 26, 2018
@jangko jangko deleted the chunked_transfer branch December 26, 2018 09:54
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.

3 participants