Second Round CURL Wrapper Review

[jdrewsen]

On Saturday, 17 December 2011 at 22:25:07 UTC, Andrei 
Alexandrescu wrote:
> On 12/2/11 10:26 PM, dsimcha wrote:
>> I volunteered ages ago to manage the review for the second 
>> round of
>> Jonas Drewsen's CURL wrapper. After the first round it was 
>> decided that,
>> after a large number of minor issues were fixed, a second 
>> round would be
>> necessary.
>>
>> Significant open issues:
>>
>> 1. Should libcurl be bundled with DMD on Windows?
>
> Yes.
>
>> 2. etc.curl, std.curl, or std.net.curl? (We had a vote a while 
>> back but
>> it was buried deep in a thread and a lot of people may have 
>> missed it:
>> http://www.easypolls.net/poll.html?p=4ebd3219011eb0e4518d35ab )
>
> std.net.curl

ok

>> Code: 
>> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
>> Docs: http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>
> I have a few comments. They are not critical for library's 
> admission (apologies I didn't make the deadline) and could be 
> integrated before the commit.
>
> 1. Tables must be centered (this is a note to self)
>
> 2. Replace d-p-l.org in examples with dlang.org

ok

> 3. "Compared to using libcurl directly this module provides a 
> simpler API for performing common tasks." -> "Compared to using 
> libcurl directly this module allows simpler client code for 
> common uses, requires no unsafe operations, and integrates 
> better with the rest of the language."

ok

> 4. From the high level ops it seems there's no way to issue a 
> PUT/POST and then direct the result to a file, or read the 
> result with a foreach.

Use byLine/Chunk with a connection initialized with post/put data:

auto http = HTTP();
http.postData = "hello world";
foreach (line; byLine("dlang.org/there", KeepTerminator.no, '\n', 
http))
   writeln(line);

Same deal for directing to file using download()

> 5. Also there seems to be no way to issue a PUT/POST from a 
> range.

Correct. It will be a later extension.

> 6. s/Examples:/Example:/

ok

> 7. s/HTTP http = HTTP("www.d-p-l.org");/auto http = 
> HTTP("www.d-p-l.org");/

ok

> 8. "receives a header or a data buffer" -> "receives a header 
> and a data buffer, respectively"

ok

> 9. "In this simple example, the headers are writting to stdout 
> and the data is ignored." How can we make it to stop the 
> request? A sentence about that would be great.

ok

> 10. "Finally the HTTP request is started by calling perform()." 
> -> "Finally the HTTP request is effected by calling perform(), 
> which is synchronous."

ok

> 11. Use InferredProtocol instead of AutoConnect(ion). The type 
> is exactly what it claims. If "inferred" is too long, 
> "AutoProtocol" would be fine too. But it's not the connection 
> that is automated. The word must not be a verb, i.e. 
> InferProtocol/DeduceProtocol would not be good.

ok

> 12. The example for InferredProtocol nee AutoConnection does 
> not show any interesting example, e.g. ftp.

improved

> 13. I think download() and upload() are good convenience 
> functions but are very limited. Such operations can last 
> arbitrarily long and it's impossible to stop them. Therefore, 
> they will foster poor program design (e.g. program hangs until 
> you kill it etc). We should keep these for convenience in 
> scripts, and we already have byXxx() for downloading, but we 
> don't have anything for uploading. We should offer output 
> ranges for uploading sync/async. Maybe in the next minor 
> release.

Using the same method as for post it is possible to set timeouts.

auto http = HTTP();
http.dataTimeout = dur!"minutes"(2);
foreach (line; byLine("dlang.org/there", KeepTerminator.no, '\n', 
http))
   writeln(line);

Regarding uploading ranges this will be handled by the same 
extension as
for POST/PUT output ranges.

> 14. Parameter doc for put(), post(), del() etc. is messed up.

ok

> 15. I don't think "connection" is the right term. For example 
> we have
>
> T[] get(Conn = AutoConnection, T = char)(const(char)[] url, 
> Conn conn = Conn());
>
> The connection is established only during the call to get(), so 
> conn is not really a connection - more like a sort of a bundle 
> of connection parameters. But then it does clean up the 
> connection once established, so I'm unsure...

But you can provide an existing connection to use has that is 
already connected to the server. Hence the name connection.


> 16. For someone who'd not versed in HTTP options, the example
>
> string content = 
> options("d-programming-language.appspot.com/testUrl2", 
> "something");
>
> isn't terribly informative.

ok - improved

> 17. In byLine: "A range of lines is returned when the request 
> is complete." This suggests the last byte has been read as the 
> client gets to the first, which is not the case. "Data will be 
> read on demand as the foreach loop makes progress." Similar for 
> byChunk.

This will change when multi API is supported as well.

> 18. In byXxxAsync wait(Duration) should be cross-referenced to 
> its definition.

ok

> =========================
>
> Overall: I think this is a valuable addition to Phobos, but I 
> have the feeling we don't have a good scenario for interrupting 
> connections. For example, if someone wants to offer a "Cancel" 
> button, the current API does not give them a robust option to 
> do so: all functions that transfer data are potentially 
> blocking, and there's no thread-shared way to interrupt a 
> connection in a thread from another thread.
>
> Such functionality may be a pure addition later, so I think we 
> can commit this as is. Please use std.net.curl.
>

They can abort from the callback handlers - but otherwise it is a 
limitation of the libcurl easy API.
To actually do this the multi extension would have to be added.

Thank you for the comments and sorry for the late reply.

/Jonas