Added additional constructor to ParserSettings that complies with the old api #49

tehrengruber · 2016-05-14T07:25:29Z

Currently a lot of packages that rely on the HttpParser fail to build because of the backward incompatible changes in the signature of the ParserSettings constructor. I've added an additional constructor to ParserSettings with the old signature that prints a deprecated warning, but gives all packages that rely on HttpParser a grace period to update.

… old api, but prints a deprecated warning

coveralls · 2016-05-14T07:39:00Z

Changes Unknown when pulling 6c4d618 on tehrengruber:master into * on JuliaWeb:master*.

coveralls · 2016-05-14T08:04:47Z

Changes Unknown when pulling 2b091a2 on tehrengruber:master into * on JuliaWeb:master*.

yuyichao · 2016-05-14T12:36:33Z

src/HttpParser.jl

+            * " on_header_field_cb, on_header_value_cb, on_header_value_cb,"
+            * " on_headers_complete_cb, on_body_cb, on_message_complete_cb,"
+            * " on_chunk_header, on_chunk_complete)", :ParserSettings)
+    dummy_cb = cfunction((parser) -> 0, HTTP_CB...)


Is the deprecation necessary?

Also I think this syntax is 0.5 only.

tehrengruber · 2016-05-14T13:17:13Z

I think the deprecation is useful. Currently a lot of packages just broke because of the signature change: e.g. HttpServer (PR pending), Requests (PR pending) and various other). After a given grace period we can remove the additional constructor safely.

tehrengruber · 2016-05-14T13:17:49Z

The syntax works without problems on 0.4 as you can see in the CI output

yuyichao · 2016-05-14T13:18:21Z

What I mean is that this doesn't need to be deprecated.

tehrengruber · 2016-05-14T13:21:29Z

Thats right, but then we should make all callback arguments optional to be consistent.

yuyichao · 2016-05-14T13:24:20Z

but then we should make all callback arguments optional to be consistent.

If any of them are reasonably optional, I think that's a good idea.

yuyichao · 2016-05-14T13:26:41Z

julia> cfunction(()->1, Int, Tuple{})
ERROR: only generic functions are currently c-callable
 in cfunction at ./c.jl:9

No depwarn is printed on the CI so I don't think it's covered.

tehrengruber · 2016-05-14T13:46:51Z

I've added another version where every callback is optional. Generally I think that all of the arguments are optional, since users of the library might only be interested in some of the callbacks. The cleanest solution would be to have the default constructor, a constructor with keyword arguments (if that works) and another constructor that is compatible with the old api but deprecated. What do you think?

coveralls · 2016-05-14T13:47:26Z

Changes Unknown when pulling 88ee6d7 on tehrengruber:master into * on JuliaWeb:master*.

yuyichao · 2016-05-14T13:49:05Z

src/HttpParser.jl

@@ -85,6 +85,34 @@ type ParserSettings
    on_message_complete_cb::Ptr{Void}
    on_chunk_header::Ptr{Void}
    on_chunk_complete::Ptr{Void}
+
+    function ParserSettings(
+      on_message_begin_cb::Ptr{Void} = Ptr{Void}(0),


yuyichao · 2016-05-14T13:53:55Z

a constructor with keyword arguments (if that works) and another constructor that is compatible with the old api but deprecated.

I think that could work. (I'm personally not a big fan of kw arguments before they become fast but I guess that isn't a good reason for not using it at all....)

yuyichao · 2016-05-14T13:55:44Z

And if this includes a API breakage (edit: i.e. use kwargs instead) (even though it comes with deprecated old version) I'd like to hear opinions from other members before merging.

coveralls · 2016-05-14T13:55:54Z

Changes Unknown when pulling ae7509e on tehrengruber:master into * on JuliaWeb:master*.

yuyichao · 2016-05-14T14:05:20Z

src/HttpParser.jl

+        dummy_cb = cfunction(dummy, HTTP_CB...)
+        new(
+          on_message_begin_cb == C_NULL ? dummy_cb : on_message_begin_cb,
+          on_url_cb == C_NULL ? dummy_cb : on_url_cb,


And I believe http_cb and http_data_cb need two different dummies (or two methods of it at least).

… old api Added additonal constructor to ParserSettings that accepts keyword arguments

tehrengruber · 2016-05-14T14:14:29Z

And I believe http_cb and http_data_cb need two different dummies (or two methods of it at least).

Absolutely I've changed that accordingly.

The last commit is what I've proposed previously

Generally I think that all of the arguments are optional, since users of the library might only be interested in some of the callbacks. The cleanest solution would be to have the default constructor, a constructor with keyword arguments (if that works) and another constructor that is compatible with the old api but deprecated. What do you think?

We now have a constructor that complies with the old api, but which is deprecated and another constructor that accepts keyword arguments. By doing so the current API is preserved, the old api is still usable but deprecated and we have a new constructor for people that only use some callbacks (unused for now).

Edit: We might want to rebase all of the commits in this pull request to keep the commit history clean.

coveralls · 2016-05-14T14:15:11Z

Changes Unknown when pulling 07540f6 on tehrengruber:master into * on JuliaWeb:master*.

yuyichao · 2016-05-14T14:23:29Z

src/HttpParser.jl

+      0
+    end
+    dummy_cb = cfunction(dummy, HTTP_CB...)
+    dummy_data_cb = cfunction(dummy, HTTP_DATA_CB...)


I don't think this is well supported on 0.4. Still better to move this out. (edit: this -> dummy, not dummy_cb and dummy_data_cb)

According to CI this works fine on 0.4. Any reason it shouldn't work? You mean I should move the function dummy outside of constructor? That would work, but will pollute the HttpParser namespace and I assume that LLVM is smart enough to make this fast anyway.

According to CI this works fine on 0.4.

The CI says absolutely nothing if the code isn't tested at all.

Any reason it shouldn't work?

The function is vararg.

You mean I should move the function dummy outside of constructor?

Yes.

That would work, but will pollute the HttpParser namespace

It's not exported.

and I assume that LLVM is smart enough to make this fast anyway.

LLVM has nothing to do with this. This is done at julia level and there's no way julia can reuse the code for the two dummies in this function and the one above.

The CI says absolutely nothing if the code isn't tested at all.

Ya I overlooked that. Just tested it with 0.4.5 and it doesn't work (even though varargs are supported), so I need to fix it.

LLVM has nothing to do with this. This is done at julia level and there's no way julia can reuse the code for the two dummies in this function and the one above. If one looks a little bit deeper one sees that cfunction actually checks the passed function for side effect.

You are right, this happens on the julia level, but I don't agree that the code can (theoretically) not be reused. If you look at this short example you will see that the generated c function pointer is always the same, which implies the opposite.

cfuncs = Ptr{Void}[] funcs = Function[] function blub() function bla() 0 end push!(cfuncs, cfunction(bla, Int, ())) push!(funcs, bla) end blub() blub() # 2-element Array{Ptr{Void},1}: # Ptr{Void} @0x00007f811b134000 # Ptr{Void} @0x00007f811b134000 println(cfuncs)

Anyway, this is a theoretical discussion and looking at the funcs array in the above example shows that additional memory is allocated whenever blub is called, so I will move dummy outside of the constructor for performance reasons.

If you look at this short example you will see that the generated c function pointer is always the same, which implies the opposite.

I mean the dummy in the two functions you added. Calling cfunction on the same function will reuse the c pointer and syntactically equivalent function definition shares the code but that's not what I mean.

coveralls · 2016-05-14T18:43:11Z

Changes Unknown when pulling 969e995 on tehrengruber:master into * on JuliaWeb:master*.

tehrengruber · 2016-05-14T18:44:31Z

Ok I think we finally made it :-) Any further comments & suggestions?

yuyichao · 2016-05-14T18:46:03Z

The change LGTM now, I'll let others comment on what the best interface should be (and therefore, if these should be inner constructors).

The added constructor should be tested though.

wildart · 2016-05-15T04:04:32Z

src/HttpParser.jl

@@ -87,6 +87,61 @@ type ParserSettings
    on_chunk_complete::Ptr{Void}
 end

+function dummy(parser)
+  0


4 spaces indent

aviks · 2017-07-14T15:19:01Z

Sorry this never got merged. Is this still relevant?

Added additional constructor to ParserSettings that complies with the…

6c4d618

… old api, but prints a deprecated warning

Fixed small typo

2b091a2

tehrengruber closed this May 14, 2016

tehrengruber reopened this May 14, 2016

yuyichao reviewed May 14, 2016
View reviewed changes

yuyichao mentioned this pull request May 14, 2016

add compatibility with new HttpParser v2.6.2 JuliaWeb/Requests.jl#119

Open

callback arguments of ParserSettings are optional now

88ee6d7

yuyichao reviewed May 14, 2016
View reviewed changes

Changed Ptr{Void}(0) to C_NULL

ae7509e

yuyichao reviewed May 14, 2016
View reviewed changes

Added additional constructor to ParserSettings that complies with the…

07540f6

… old api Added additonal constructor to ParserSettings that accepts keyword arguments

yuyichao reviewed May 14, 2016
View reviewed changes

Moved dummy function outside of ParserSettings constructor

969e995

tehrengruber closed this May 14, 2016

tehrengruber reopened this May 14, 2016

wildart reviewed May 15, 2016
View reviewed changes

Fixed indentation

f3853bc

aviks closed this Sep 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added additional constructor to ParserSettings that complies with the old api #49

Added additional constructor to ParserSettings that complies with the old api #49

tehrengruber commented May 14, 2016

coveralls commented May 14, 2016 •

edited

coveralls commented May 14, 2016 •

edited

yuyichao May 14, 2016

yuyichao May 14, 2016

tehrengruber commented May 14, 2016

tehrengruber commented May 14, 2016

yuyichao commented May 14, 2016

tehrengruber commented May 14, 2016

yuyichao commented May 14, 2016

yuyichao commented May 14, 2016

tehrengruber commented May 14, 2016

coveralls commented May 14, 2016 •

edited

yuyichao May 14, 2016

yuyichao commented May 14, 2016

yuyichao commented May 14, 2016 •

edited

coveralls commented May 14, 2016 •

edited

yuyichao May 14, 2016

tehrengruber commented May 14, 2016 •

edited

coveralls commented May 14, 2016 •

edited

yuyichao May 14, 2016 •

edited

tehrengruber May 14, 2016 •

edited

yuyichao May 14, 2016

tehrengruber May 14, 2016

yuyichao May 14, 2016 •

edited

coveralls commented May 14, 2016 •

edited

tehrengruber commented May 14, 2016

yuyichao commented May 14, 2016

wildart May 15, 2016

aviks commented Jul 14, 2017

Added additional constructor to ParserSettings that complies with the old api #49

Added additional constructor to ParserSettings that complies with the old api #49

Conversation

tehrengruber commented May 14, 2016

coveralls commented May 14, 2016 • edited

coveralls commented May 14, 2016 • edited

yuyichao May 14, 2016

Choose a reason for hiding this comment

yuyichao May 14, 2016

Choose a reason for hiding this comment

tehrengruber commented May 14, 2016

tehrengruber commented May 14, 2016

yuyichao commented May 14, 2016

tehrengruber commented May 14, 2016

yuyichao commented May 14, 2016

yuyichao commented May 14, 2016

tehrengruber commented May 14, 2016

coveralls commented May 14, 2016 • edited

yuyichao May 14, 2016

Choose a reason for hiding this comment

yuyichao commented May 14, 2016

yuyichao commented May 14, 2016 • edited

coveralls commented May 14, 2016 • edited

yuyichao May 14, 2016

Choose a reason for hiding this comment

tehrengruber commented May 14, 2016 • edited

coveralls commented May 14, 2016 • edited

yuyichao May 14, 2016 • edited

Choose a reason for hiding this comment

tehrengruber May 14, 2016 • edited

Choose a reason for hiding this comment

yuyichao May 14, 2016

Choose a reason for hiding this comment

tehrengruber May 14, 2016

Choose a reason for hiding this comment

yuyichao May 14, 2016 • edited

Choose a reason for hiding this comment

coveralls commented May 14, 2016 • edited

tehrengruber commented May 14, 2016

yuyichao commented May 14, 2016

wildart May 15, 2016

Choose a reason for hiding this comment

aviks commented Jul 14, 2017

coveralls commented May 14, 2016 •

edited

coveralls commented May 14, 2016 •

edited

coveralls commented May 14, 2016 •

edited

yuyichao commented May 14, 2016 •

edited

coveralls commented May 14, 2016 •

edited

tehrengruber commented May 14, 2016 •

edited

coveralls commented May 14, 2016 •

edited

yuyichao May 14, 2016 •

edited

tehrengruber May 14, 2016 •

edited

yuyichao May 14, 2016 •

edited

coveralls commented May 14, 2016 •

edited