Rewritten http package for RFC 0033 #1563

pdtwonotes · 2017-02-07T04:06:25Z

This is my rewrite of the net/http package, as described in RFC 33.　　It also includes examples/httpget and examples/httpserver. The server example is quite limited and just shows that it works. A much more elaborate example can be found at pdtwonotes/tokara in the httpng branch.

SeanTAllen · 2017-02-07T04:20:29Z

@pdtwonotes this needs to be rebased against master. Do you know how to do that? If not, ping me on IRC and I can help.

SeanTAllen · 2017-02-07T04:44:01Z

@pdtwonotes there's a lot of non-standard formatting. i commented on a decent chunk of it and eventually stopped because my browser was having a hard time. i think enough should be covered to hit pretty much all the points.

i'll take a look later on after that is cleaned up for other issues.

two things i noticed, one of which i commented on.

there's some string concatention and that can be slow, i'd avoid that if it gets called often. there's a pony pattern for that.
you use error in some places. that can be slow if you expect it to get called a lot. if there are areas where it does get called a lot, can you highlight them and we can discuss if a union type would be better?

I'm really excited about replacing the old http server. Thank you for taking this on.

pdtwonotes · 2017-02-07T04:54:38Z

Where do I find these comments? And I thought I did rebase it!

SeanTAllen · 2017-02-07T04:21:51Z

examples/httpget/httpget.pony

+    // The positional parameter is the URL to be fetched.
+    let urlref = try env.args( env.args.size() - 1 ) else "" end
+    let url = try URL.valid(urlref)
+      else


standard library formatting would be for this else and other blocks to be on the same indentation level as the let

SeanTAllen · 2017-02-07T04:22:29Z

examples/httpget/httpget.pony

+      end
+
+    // The positional parameter is the URL to be fetched.
+    let urlref = try env.args( env.args.size() - 1 ) else "" end


standard library formatting would be to not have the spaces here.

(env.args.size() - 1)

SeanTAllen · 2017-02-07T04:23:02Z

examples/httpget/httpget.pony

+      "  --user (-u)  Username for authenticated queries\n" +
+      "  --pass (-p)  Password for authenticated queries\n" +
+      "  --output (-o) Name of file to write response body\n"
+      )


standard library formatting would be for the ) to be at the end of line 56.

SeanTAllen · 2017-02-07T04:23:28Z

examples/httpget/httpget.pony

+  let _env: Env
+
+  new create( env: Env, url: URL, user: String, pass: String,
+    output: String ) =>


standard library formatting would be for the => to be on the next line

I do not see this happening very often. What is the rule?

new create( env: Env, url: URL, user: String, pass: String, output: String ) =>

when wrapping but this line is actually less than 80 characters and should be

new create(env: Env, url: URL, user: String, pass: String, output: String ) =>

@SeanTAllen you forgot to remove the space before the ) in your example - just to let @pdtwonotes know.

@jemc i did. man this PR, it makes my browser so slow when I'm on the review page and I type something and there is a second delay before anything shows up. I'm surprised there aren't more I Fubar'd.

I rewrote nearly every file in the package, so it is big.

SeanTAllen · 2017-02-07T04:24:03Z

examples/httpget/httpget.pony

+      // The Notify Factory will create HTTPHandlers as required.  It is
+      // done this way because we do not know exactly when an HTTPSession
+      // is created - they can be re-used.
+      let dumpMaker = recover val NotifyFactory.create( this ) end


create(this)

SeanTAllen · 2017-02-07T04:45:03Z

packages/net/http/server.pony

    """
    _listen.dispose()
+    for conn in _sessions.values() do
+      conn.dispose()
+      end


SeanTAllen · 2017-02-07T04:45:54Z

packages/net/http/server.pony

-  for new connections. Existing connections continue to use the old routes.
+  Runs an HTTP server.
+
+### Server operation


formatting is off for this section. actor/class comments get indented

SeanTAllen · 2017-02-07T04:46:41Z

packages/net/http/http_notify.pony

+    an actor to deal with subsequent information pertaining to this
+    message.
+    """
+  fun ref chunk( data: ByteSeq val ) => None


should be newlines between each function

SeanTAllen · 2017-02-07T04:47:06Z

packages/net/http/http_notify.pony

+    essential fields filled in at this point, because ownership is being
+    transferred to the session actor.  This begins an outbound message.
+    """
+  be finish()


should be newline between each method

SeanTAllen · 2017-02-07T04:47:29Z

packages/net/http/discard_log.pony

@@ -2,5 +2,6 @@ primitive DiscardLog
  """
  Doesn't log anything.
  """
-  fun val apply(ip: String, request: Payload val, response: Payload val) =>
+  fun val apply(ip: String, transferred: USize,
+    request: Payload val, response: Payload val) =>


SeanTAllen · 2017-02-07T12:41:42Z

@pwdnotes sorry i forgot to hit submit for the comments.

SeanTAllen · 2017-02-07T12:42:15Z

apparently only 1 file is in conflict as you can see below.

Theodus · 2017-02-07T14:33:30Z

packages/net/http/mimetypes.pony

+    """
+    try
+      // This will fail if no period is found.
+      let dotpos = (name.rfind( ".", -1, 0 )+1).usize()


Standard library formatting would have no spaces between the arguments and the parentheses. Spaces should also be added around the addition so that it looks like (name.rfind(".", -1, 0) + 1).usize()

Theodus · 2017-02-07T14:38:34Z

packages/net/http/payload.pony

  let _headers: Map[String, String] = _headers.create()
-  let _body: Array[ByteSeq] = _body.create()
+  var _body: Array[ByteSeq val] = _body.create()


var field is never reassigned and is only initialized with a constructor, so it should be declared with embed.

Theodus · 2017-02-07T14:41:01Z

packages/net/http/payload.pony

-      let h = handler as ResponseHandler
-      h(consume this, Payload.response(StatusInternalServerError))
-    end
+None /*    try


indentation of None

Theodus · 2017-02-07T14:46:20Z

packages/net/http/payload.pony

+    """
+    Write the response-specific parts of an HTTP message.
+    """
+    conn.write(proto + " " + status.string() + " " + method + "\r\n")


String concatenation using + here can be slow since it allocates a new string on every +

pdtwonotes · 2017-02-08T17:02:17Z

I think the failing test is actually in error. The old version of URL.decode would decode "+" as itself, whereas in a URL "+" should decode as space. I fixed that, and that is what is breaking test test.

killerswan · 2017-02-15T20:50:27Z

Note #1543 for a version bump / release.

SeanTAllen reviewed Feb 7, 2017

View reviewed changes

Theodus reviewed Feb 7, 2017

View reviewed changes

pdtwonotes added 2 commits February 8, 2017 08:29

Rewrite of net/http as proposed in RFC 0033

8cf6296

Revoke changes to url.pony

1db71ee

Update URL decode test to match correct beahvior

2c09e19

sylvanc merged commit a71d834 into ponylang:master Feb 15, 2017

Theodus mentioned this pull request Feb 20, 2017

RFC: HTTP Streaming #1548

Closed

pdtwonotes deleted the httpng branch February 23, 2017 04:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewritten http package for RFC 0033 #1563

Rewritten http package for RFC 0033 #1563

pdtwonotes commented Feb 7, 2017

SeanTAllen commented Feb 7, 2017

SeanTAllen commented Feb 7, 2017

pdtwonotes commented Feb 7, 2017 •

edited

Loading

SeanTAllen Feb 7, 2017

SeanTAllen Feb 7, 2017

SeanTAllen Feb 7, 2017

SeanTAllen Feb 7, 2017

pdtwonotes Feb 7, 2017

SeanTAllen Feb 7, 2017

jemc Feb 7, 2017 •

edited

Loading

SeanTAllen Feb 7, 2017

pdtwonotes Feb 7, 2017

SeanTAllen Feb 7, 2017

SeanTAllen Feb 7, 2017

SeanTAllen Feb 7, 2017

SeanTAllen Feb 7, 2017

SeanTAllen Feb 7, 2017

SeanTAllen Feb 7, 2017

SeanTAllen commented Feb 7, 2017

SeanTAllen commented Feb 7, 2017

Theodus Feb 7, 2017

Theodus Feb 7, 2017

Theodus Feb 7, 2017

Theodus Feb 7, 2017

pdtwonotes commented Feb 8, 2017

killerswan commented Feb 15, 2017

Rewritten http package for RFC 0033 #1563

Rewritten http package for RFC 0033 #1563

Conversation

pdtwonotes commented Feb 7, 2017

SeanTAllen commented Feb 7, 2017

SeanTAllen commented Feb 7, 2017

pdtwonotes commented Feb 7, 2017 • edited Loading

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

jemc Feb 7, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Feb 7, 2017

SeanTAllen commented Feb 7, 2017

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

pdtwonotes commented Feb 8, 2017

killerswan commented Feb 15, 2017

pdtwonotes commented Feb 7, 2017 •

edited

Loading

jemc Feb 7, 2017 •

edited

Loading