Curl wrapper

[Jonas Drewsen]

On 17/05/11 06.43, Andrei Alexandrescu wrote:
> On 05/16/2011 04:07 PM, jdrewsen wrote:
>> Hi,
>>
>> I've been working on a wrapper for the etc.c.curl module. It is now
>> pretty stable and I would very much like some feedback on the API.
>>
>> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>>
>> BTW I use template mixins which doesn't seem to get included in the
>> generated docs. Is there any way I can make this work?
>>
>> /Jonas
>
> Now that's one sight for sore eyes. Nice work. A few comments and nits:
>
> 1. Http can't be a class. Network resources are the poster child of
> expensive resources that are NOT memory and don't work well with garbage
> collection. RAII is the poster child of deterministic resource
> management. So...

I see what you mean. Will fix.

> 2. Furthermore I see customization via delegates (onReceive etc). That
> should be a good proxy for customization instead of using inheritance.

The code actually defined the Http/Ftp classes as final. But somehow it 
is not reflected in the generated docs. Anyway, I will change it to 
struct and inheritance is not an issue anyway.


> 3. Data on wire is ubyte[], not void[].

Data received is ubyte[] and a convenience method for conversion to 
string is provided.

Data send is void[] since all arrays implicitely converts to void[]. 
This makes e.g [1,2,3,4,5] works as input. Convenience methods are 
provided for chars in order to set HTTP headers correctly.

For onSend callback it is simply void[] in order to use it as an out 
parameter.

For Http.post/put it is const(void)[]
For Http.postAsync/putAsync it is immutable(void)[] because internally I 
need to send the data to another thread using message passing.

Hopes this makes sense?


> 4. Shouldn't onSend take a ref to its argument in case it wants to
> expand it?

It is not allowed to expand it. The callback originates from libcurl 
which determines the size of the array.

> 5. Why HttpMethod and not Http.Method?

I had the thought myself but decided on the former. By looking at the 
replies to my post it seems that it should be changed. I'll do that.

> 6. "Do not use the same instance of this class in two threads
> simultanously." -> remove. A D program isn't supposed to do that unless
> you offer shared methods.

Ok. So even though I declare:

shared Http http = new Http();

I cannot have multiple threads call http.perform without that method 
being marked as shared as well?

If thats the case I'll remove the comment.

> 7. HttpStatusLine -> Http.StatusLine. One up for encapsulation.
>
> 8. HttpMethod -> Http.Method. Ditto.
>
> 9. "this(in const(char)[] url);" -> "this(string url);" I assume you
> store a copy of the URL inside by calling e.g. to!string. If the client
> has a string, you refuse to tap into that nice information and share the
> string safely and efficiently, and make a copy of it for no good reason.
> If the client doesn't have a string, no worry, the compiler will tell
> her she needs to do a copy.

I do not store a copy but calls libcurl which in turn does a copy by 
itself. So I believe the current signature is appropriate?

> 10. addHeader etc. Same comment. Use string whenever you want to keep a
> copy anyway.

At 9. the string is copied by libcurl itself.

> 11. setTimeCondition -> use core.Duration for time representation
> throughout.

Will fix.

> 12. AsyncHttpResult -> Http.AsyncResult :o)
>
> 13. HttpResult -> Http.Result
>
> 14. Isn't the max redirect configurable via a parameter?

Yes. It is called Http.followLocation from libcurls followLocation 
option. I will rename it to maxRedirections for clearity.

> 15. Typo: "Callbacks is not supported..."

ok.

> 16. Documentation should point to descriptions of the HTTP methods
> wrapped (e.g. "post", "del" etc).

Do you mean point to the relevant RFC?

> 17. There should be an example with login/password.

ok.

> 18. See onReceiveHeader">std.curl.Curl.onReceiveHeader and others look
> like doc macros gone wrong.
>
> 19. "chuncked" -> "chunked"
>
> 20. "Max allowed redirs. -1 for infinite." -> use uint and uint.max for
> infinite.

Will fix.

> 21. "short isRunning()" -> what's the semantics of the short?

This is a bug. It returns an internal bool value.

> 22. byLine/byChunk should not expose a string or generally data that can
> be shared safely by the client. That's because it would need to create a
> new string with each iteration, which is very inefficient. Instead, they
> should expose char[]/ubyte[] just like File.byLine does, and reuse the
> buffer with each call. Essentially byLine/byChunk should be
> near-zero-overhead means to transfer and inspect arbitrarily large streams.

byLine/byChunk is currenly only available for the async methods which is 
implemented using message passing. This means that for passing the 
message a copy has to be made because a message is by-value or immutable 
data. Is there another way for me to pass the message without doing a 
copy... some kind of move semantic?

> 23. Ftp should have get in-memory and streaming byLine/byChunk, not only
> get to file.

Definitely. I wanted to get feedback on HTTP API before doing the same 
to FTP.

> 24. The shutdown mechanism should be handled properly. Shutting down
> libcurl would have all pending transfers instantly throw a special
> exception. Without a shutdown API, applications won't be able to
> implement e.g. a Quit button.

If you're only using this API this should be handled. But I see that it 
is not documented.

> Again, great work!

Thank you for the feedback.