CURL review request

[Jonas Drewsen]

On 17/08/11 00.21, Jonathan M Davis wrote:
> On Tuesday, August 16, 2011 12:32 Martin Nowak wrote:
>> On Tue, 16 Aug 2011 20:48:51 +0200, jdrewsen<jdrewsen at nospam.com>  wrote:
>>> Den 16-08-2011 18:55, Martin Nowak skrev:
>>>> On Tue, 16 Aug 2011 15:13:40 +0200, dsimcha<dsimcha at yahoo.com>  wrote:
>>>>> On 8/16/2011 7:48 AM, Jonas Drewsen wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> This is a review request for the curl wrapper. Please read the "known
>>>>>> issues" in the top of the source file and if possible suggest a
>>>>>> solution.
>>>>>>
>>>>>> We also need somebody for running the review process. Anyone?
>>>>>>
>>>>>> Code:
>>>>>> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
>>>>>> Docs:
>>>>>> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>>>>>>
>>>>>> Demolish!
>>>>>>
>>>>>> /Jonas
>>>>>
>>>>>  From a quick look, this looks very well thought out. I'll review it
>>>>> more thoroughly when I have more time. A few questions/comments from a
>>>>> quick look at the docs:
>>>>>
>>>>> Does the async stuff use D threads, or does Curl have its own async
>>>>> API?
>>>>>
>>>>> In your examples for postData, you have onReceive a ubyte[] and write
>>>>> it out to console. Did you mean to cast this to some kind of string?
>>>>>
>>>>> For onReceive, what's the purpose of the return value?
>>>>>
>>>>> If/when this module makes it into Phobos, are we going to start
>>>>> including a libcurl binary with DMD distributions so that std.curl
>>>>> feels truly **standard** and requires zero extra configuration?
>>>>
>>>> I was also wondering about the async handling. In the long-term I'd like
>>>> to see a bigger picture for async handling in phobos (offering some kind
>>>> of futures, maybe event-loops etc.).
>>>> Though this is not a requirement for the curl wrapper now.
>>>> std.parallelism also has some kind of this stuff and file reading would
>>>> benefit from it too.
>>>
>>> This has been discussed before and I also think this is very important.
>>> But before that I think some kind of package management should be
>>> prioritized (A DIP11 implementaion or a more traditional solution).
>>>
>>>> One thing I spotted at a quick glance, sending to be filled buffers to
>>>> another thread should not be done by casting to shared not immutable.
>>>
>>> I'm not entirely sure what you mean. There is no use of shared buffers
>>> in the wrapper. I do cast the buffer between mutable/immutable because
>>> only immutable or by value data can be passed using std.concurrency.
>>> Since the buffers are only used by the thread that currently has the
>>> buffer this is safe. I've previously asked for a non-cast solution (ie.
>>> some kind of move between threads semantic for std.concurrency) but was
>>> advised that this was the way to do it.
>>>
>>>> martin
>>
>> Pardon the typo. What I meant is that AFAIK casting from immutable to
>> mutable has undefined behavior.
>> The intended method for sending a uint[] buffer to another thread is to
>> cast that
>> buffer to shared (cast(shared(uint[])) and casting away the shared on the
>> receiving side.
>> It is allowed to send shared data using std.concurrency.
>
> Casting away immutability and then altering data is undefined. Actually
> casting it away is defined. So, if you have data in one thread that you know
> is unique, you can cast it to immutable (or std.exception.assumeUnique to do
> it) and then send it to another thread. On that thread, you can then cast it
> to mutable and alter it.
>
> However, you're circumventing the type system when you do this. So, you have
> to be very careful. You're throwing away the guarantees that the compiler
> makes with regards to const and immutable. It _is_ guaranteed to work though.
> And I'm not sure that there's really any difference between casting to shared
> and back and casting to immutable and back. In both cases, you're
> circumventing the type system. The main difference would be that if you
> screwed up with immutable and cast away immutable on something that really was
> immutable rather than something that you cast to immutable just to send it to
> another thread, then you could a segfault when you tried to alter it, since it
> could be in ROM.
>
> - Jonathan M Davis

Yeah I know you have to be careful when doing these kind of things. I 
ran into the problem of sending buffers between threads (using 
std.concurrency) so that they could be reused. There isn't any "move 
ownership" support in place so Andrei suggested I could do it by casting 
immutable.

If anyone knows of a cleaner way to do this please tell.

/Jonas