CURL review request

jdrewsen jdrewsen at nospam.com
Fri Aug 19 13:07:39 PDT 2011


Den 19-08-2011 00:55, Timon Gehr skrev:
> On 08/18/2011 11:33 PM, jdrewsen wrote:
>> Den 17-08-2011 18:21, Timon Gehr skrev:
>>> On 08/17/2011 05:58 PM, Jonathan M Davis wrote:
>>>> On Wednesday, August 17, 2011 11:30:06 Steven Schveighoffer wrote:
>>>>> On Wed, 17 Aug 2011 11:05:56 -0400, jdrewsen<jdrewsen at nospam.com>
>>>>> 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?
>>>>>
>>>>> Once it's immutable, it can never be mutable again. Casting to
>>>>> immutable
>>>>> is a one-way street. Yes, you can cast to mutable, but you still can't
>>>>> change the data unless you want undefined behavior.
>>>>>
>>>>> Shared is not like that, an item can be thread-local, then shared,
>>>>> then
>>>>> thread local again, all the time being mutable. It also reflects
>>>>> better
>>>>> what the process is (I'm sharing this data with another thread, then
>>>>> that
>>>>> thread is taking ownership). There's still the possibility to screw
>>>>> up,
>>>>> but at least you are not in undefined territory in the
>>>>> correctly-implemented case.
>>>>
>>>> Are you sure? As I understand it, there's no real difference between
>>>> casting to
>>>> immutable and back and casting to shared and back. Both circumvent the
>>>> type
>>>> system. In the one case, the type system guarantees that the data
>>>> can't be
>>>> altered, and you're breaking that guarantee, because you know that it
>>>> _can_
>>>> be, since you created the data and know that it's actually mutable.
>>>
>>> No. As soon as the data is typed as immutable anywhere it cannot be
>>> changed anymore. You only break guarantees if you actually try to change
>>> the data (otherwise std.typecons.assumeUnique would perform its job
>>> outside defined behavior btw)
>>
>> I'm thinking down the same lines as Jonathan. Is the behavior for
>> immutable casts that you describe specified in the language reference
>> somewhere?
>>
>> I have no problem with using shared casts instead of immutable - I just
>> want make sure it is really needed.
>>
>
> Yes.
>
> http://www.digitalmars.com/d/2.0/const3.html
> See 'removing immutable with a cast'.
>
> It basically says that your program is in error if it changes data whose
> immutability has been cast away.
>
> If it's 'really needed' depends on what you consider 'really needed'. It
> will work as intended with immutable (and the current DMD compiler
> implementation and probably most implementations of the language that
> there will be), but the code will still be incorrect.
>

I don't think the documentation is unambiguous in this matter. Anyway I 
tried to change it to shared but it doesn't seem to work:

Works:
     workerTid.send(cast(immutable(char)[])arr);

Doesn't work:
     workerTid.send(cast(shared(char)[])arr);

.../std/variant.d(528): Error: function core.stdc.string.memcpy (void* 
s1, in const(void*) s2, uint n) is not callable using argument types 
(ubyte[24u]*,shared(char[])*,uint)
.../std/variant.d(528): Error: cannot implicitly convert expression (& 
rhs) of type shared(char[])* to const(void*)
make: *** [all] Error 1

I don't know if this is a bug in std.variant?

/Jonas





More information about the Digitalmars-d mailing list