http: chunked transfer encoding #174

legendecas · 2018-07-03T14:26:35Z

Expect resolving #133

http client:

default encoding of http request shall be chunked
wrap http chunk with chunk header and a \r\n trailing

http server:

default encoding of response shall be chunked
wrap http chunk with chunk header and a \r\n trailing

all tests pass
test case over chunked encoding
document

yorkie · 2018-07-04T07:45:25Z

src/js/http_client.js

+  this.path = options.path || '/';
+
+  var methodIsString = (typeof method === 'string');
+  if (methodIsString && method) {


I think we just defines all methods and check by methods[method] here.

Actually it is possible to use a custom method rather than most common methods like GET, POST in http request.
#ref: https://tools.ietf.org/html/rfc2616#section-5.1.1

I think we don't need to support the whole context-free grammar for Method, a reduced CFG is:

Method = "OPTIONS" | "GET" | "HEAD" | "POST" | "PUT" | "DELETE" | "TRACE" | "CONNECT"

An origin server SHOULD return the status code 405 (Method Not Allowed) if the method is known by the origin server but not allowed for the requested resource, and 501 (Not Implemented) if the method is unrecognized or not implemented by the origin server.

As the above description, we could respond a 501 for those methods is invalid.

In my opinion, if a custom method shall be considered as supported or not is up to servers rather than an client library since it's valid by definition.

Hmm, really think there are not too much developers to use the custom feature. But let's support it, that's reasonable than me.

yorkie · 2018-07-04T08:00:46Z

src/js/http_client.js

@@ -206,6 +220,13 @@ function responseOnEnd() {
  }
 }

+ClientRequest.prototype._implicitHeader = function _implicitHeader() {
+  if (this._header) {
+    throw new Error('Cannot render headers after they are sent');


This code seems say: if header does not exist, it's unable to render again, because it can be sent? Why the this._header means the response has been sent?

yorkie · 2018-07-04T08:03:17Z

src/js/http_outgoing.js

@@ -17,14 +17,18 @@
 var util = require('util');
 var stream = require('stream');

+var crlf_buf = Buffer.from('\r\n');


Use crlf or crlfBuf?

yorkie · 2018-07-04T08:04:51Z

src/js/http_outgoing.js


-  return ret;
+    this._send(len.toString(16), 'latin1', null);


We might have no "latin1" encoding yet.

An alias of latin1 is binary, which is one-byte encoded.
I've tested over this, data have been correctly encoded.

Nope, see the following function: https://github.com/Rokid/ShadowNode/blob/3d408ea40191c693e470a9d0a48be60a98b84f38/src/js/net.js#L160-L165

It shows the encoding latin1 did not be used.

looks like it doesn't support specify encoding anyway. but for better compatibility in the future implementations, it's reasonable to leave as it is.

The write(chunk, callback) only has 2 arguments, however we pass it with 3 arguments. Therefore this makes confusion and inconsistent, let's do that after we changed the function is 100% same as Node.js.

Maybe we support encoding in .write(chunk, encoding, callback) method before this?

😅Actually the chunk header is a hex number so that's ok to use default ascii encoding.
It's ok to remove the encoding argument at this scenario.

yorkie · 2018-07-04T08:06:58Z

src/js/http_outgoing.js


 function OutgoingMessage() {
  stream.Stream.call(this);

  this.writable = true;

  this._hasBody = true;
-  this._bodyLength = 0;
+  this._contentLength = null;


Why removing the _bodyLength?

It's renamed for align with naming of the header Content-Length.

Good explain! 👍

yorkie

Just leave some comments.

yorkie · 2018-07-05T05:51:55Z

src/js/http_outgoing.js


-  return ret;
+    this._send(len.toString(16), 'latin1', null);


Nope, see the following function: https://github.com/Rokid/ShadowNode/blob/3d408ea40191c693e470a9d0a48be60a98b84f38/src/js/net.js#L160-L165

It shows the encoding latin1 did not be used.

yorkie · 2018-07-09T09:20:44Z

How is this in progress @legendecas? Currently I think this should be lack of only the documents, because of #184 did fix the encoding params.

… is chunked

… together

…oing to be sent

legendecas · 2018-07-09T11:17:51Z

Document is updated! 🤠

yorkie · 2018-07-09T12:05:13Z

Merged @legendecas :)

legendecas changed the title ~~http: chunked transfer encoding~~ [WIP] http: chunked transfer encoding Jul 3, 2018

yorkie added the http label Jul 3, 2018

legendecas force-pushed the http-chunked-encoding branch from f140c00 to fb78ff6 Compare July 3, 2018 15:28

legendecas requested review from yorkie, algebrait and qile222 July 4, 2018 05:22

yorkie reviewed Jul 4, 2018

View reviewed changes

legendecas changed the title ~~[WIP] http: chunked transfer encoding~~ http: chunked transfer encoding Jul 5, 2018

yorkie requested changes Jul 5, 2018

View reviewed changes

legendecas added 6 commits July 9, 2018 17:23

feat: wrap chunk with chunk header and an \r\n if transfer encoding…

203f737

… is chunked

fix: the content-length and transfer-encoding header must not be sent…

be2fbff

… together

fix(http-tests): headers shall be rendered just before first packet g…

170363e

…oing to be sent

test(http): chunked transfer encoding

d21aa28

style: naming of variables

8c5b0f7

doc: http chunked encoding

75b06e2

legendecas force-pushed the http-chunked-encoding branch from 3d408ea to 75b06e2 Compare July 9, 2018 11:13

yorkie approved these changes Jul 9, 2018

View reviewed changes

yorkie merged commit 1226ad5 into master Jul 9, 2018

yorkie deleted the http-chunked-encoding branch July 9, 2018 12:04

yorkie pushed a commit that referenced this pull request Jul 10, 2018

http: support chunked transfer encoding (#174)

b4e5c38

yorkie mentioned this pull request Jul 10, 2018

http: support chunked transfer-encoding #133

Closed

yorkie mentioned this pull request Jul 27, 2018

Release Proposal: working on v0.9.x #227

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: chunked transfer encoding #174

http: chunked transfer encoding #174

legendecas commented Jul 3, 2018 •

edited

yorkie Jul 4, 2018

legendecas Jul 5, 2018

yorkie Jul 5, 2018 •

edited

legendecas Jul 5, 2018

yorkie Jul 5, 2018

yorkie Jul 4, 2018

yorkie Jul 4, 2018

yorkie Jul 4, 2018

legendecas Jul 5, 2018

yorkie Jul 5, 2018 •

edited

legendecas Jul 5, 2018

yorkie Jul 5, 2018

yorkie Jul 5, 2018 •

edited

legendecas Jul 5, 2018 •

edited

yorkie Jul 4, 2018

legendecas Jul 4, 2018

yorkie Jul 4, 2018

yorkie left a comment

yorkie Jul 5, 2018 •

edited

yorkie commented Jul 9, 2018

legendecas commented Jul 9, 2018 •

edited

yorkie commented Jul 9, 2018

http: chunked transfer encoding #174

http: chunked transfer encoding #174

Conversation

legendecas commented Jul 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorkie Jul 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorkie Jul 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorkie Jul 5, 2018 • edited

Choose a reason for hiding this comment

legendecas Jul 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorkie left a comment

Choose a reason for hiding this comment

yorkie Jul 5, 2018 • edited

Choose a reason for hiding this comment

yorkie commented Jul 9, 2018

legendecas commented Jul 9, 2018 • edited

yorkie commented Jul 9, 2018

legendecas commented Jul 3, 2018 •

edited

yorkie Jul 5, 2018 •

edited

yorkie Jul 5, 2018 •

edited

yorkie Jul 5, 2018 •

edited

legendecas Jul 5, 2018 •

edited

yorkie Jul 5, 2018 •

edited

legendecas commented Jul 9, 2018 •

edited