CURL review request

Thu Aug 18 17:41:48 PDT 2011

On Thursday, August 18, 2011 16:50 Jonathan M Davis wrote:
> On Thursday, August 18, 2011 16:00 Timon Gehr wrote:
> > On 08/19/2011 12:35 AM, Jonathan M Davis wrote:
> > > On Thursday, August 18, 2011 14:33 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.
> > > 
> > > The behavior of casting a way const or immutable on a value and then
> > > mutating it is undefined by the language, because you're breaking the
> > > language's guarantees and what happens depends entirely on whether the
> > > actual object was actually immutable. However, in the case of casting
> > > to immutable and then casting back, you _know_ that the object is
> > > mutable, so there's no problem. You're just circumventing the type
> > > system which throws away the guarantees that it gives you about
> > > immutability, which could screw up optimizations if you had actually
> > > did more than just pass the variable around. But that's just not
> > > happening here.
> > > 
> > > As for casting to and from shared and mutating the object, I don't see
> > > how it is any more defined than casting to and from immutable and then
> > > mutating the object is. In both cases, you circumvented the type
> > > system, which breaks the compiler's guarantees and risks bugs if you
> > > actually do more than just pass the variable around before casting it
> > > back to being thread-local and mutable.
> > > 
> > > - Jonathan M Davis
> > 
> > As long as the data is not being shared between multiple threads after
> > it's sharedness has been cast away, you are well in defined area,
> > because you are NOT breaking anything.
> 
> The _only_ reason that you're not breaking anything is because you are
> being careful and making sure that the data is not actually shared between
> threads. You're _still_ circumventing the type system and breaking the
> guarantee that a non-shared variable is not shared between threads. _You_
> are the one guaranteeing that the variable is only on one thread, not the
> compiler. And when you cast away immutable, _you_ are the one guaranteeing
> that immutable data is not being mutated, not the compiler. And in this
> case, you can make that guarantee in exactly the same way that you can
> guarantee that the variable which was cast to shared and back to be passed
> across threads isn't actually shared between threads once it has been
> passed.
> 
> > The crucial difference between immutable and shared is, that something
> > that is immutable will always be immutable, but being shared or not may
> > change dynamically.
> > 
> > Casting to immutable is a one-way-street, while casting to shared is not.
> 
> Casting to immutable does not make the data magically immutable. It makes
> it so that the compiler treats it as immutable. Casting from immutable
> makes it so that the compiler treats it as mutable. It does not alter
> whether the data is actually immutable. Casting away immutable and
> altering data is undefined, because it depends entirely on whether the
> data is actually immutable or not. If it isn't, and you don't have any
> other references to the data (or none of the other references are
> immutable), then you don't break _anything_ by mutating the variable after
> having cast away immutable.
> 
> With both shared and immutable, casting it away is circumnventing the type
> system. In both cases, _you_ must make sure that you code in a way which
> does not violate the guarantees that the compiler normally makes about
> those types. If you do violate those guarantees (e.g. by sharing
> non-shared data across threads or by mutating data which really was
> immutable), then you have a bug, and what happens is dependent on the
> compiler implementation and on your code. But if you don't violate those
> guarantees, then your program is fine. It's the same for both shared and
> immutable. It's just that the bugs that you're likely to get when you
> violate those guarantees are likely to be quite different.

Now, I'll buy an argument that it's better to cast to shared and back with 
send than to use immutable based on the grounds that what you're doing doesn't 
really have anything to do with mutability aside from the fact that that's one 
way to get send to accept the data. What you're really doing is trying to pass 
ownership, which isn't really modeled at all in D's present type system, but 
because it involves sharing data between threads (even if it isn't really 
staying shared between threads), it's at least conceptually closer to making 
it shared than it is to making it immutable - not to mention that immutable is 
shared _anyway_. Also, it's less error-prone, since at worst, you'll make the 
mistake of passing data which is still actually shared between threads 
(instead of the ownership being transferred), causing race conditions and 
whatnot, whereas with immutable not only has that same problem, but you could 
also accidentally send data which is actually immutable, which could be really 
nasty depending on the exact effects of mutating immutable data (a segfault 
seems likely).

So, I can accept that it's better to cast to shared to pass ownership of an 
object from one thread to another via send rather than casting to immutable to 
do that, but I still hold that there's really no difference between using 
shared and using immutable in that you're circumventing the type system and 
using undefined behavior in both cases.

- Jonathan M Davis