CURL review request

[jdrewsen]

Den 17-08-2011 15:51, Steven Schveighoffer skrev:
> On Wed, 17 Aug 2011 05:43:00 -0400, Jonas Drewsen <jdrewsen at nospam.com>
> wrote:
>
>> 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.
>
> casting to shared and back. Passing shared data should be supported by
> std.concurrency, and casting away shared is defined as long as you know
> only one thread owns the data after casting.
>
> -Steve

Why is this cleaner than casting to immutable and back?

/Jonas