CURL review request

Timon Gehr timon.gehr at gmx.ch
Wed Aug 17 08:26:31 PDT 2011


On 08/17/2011 05:05 PM, jdrewsen wrote:
> 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
>
>
>

Because immutable gives strictly stronger guarantees than shared. 
Casting away immutable and altering the resulting data is incorrect, 
even if it works with a particular implementation of the language.
Casting away shared is correct iff the data is owned by at most one 
thread after the cast.


More information about the Digitalmars-d mailing list