ResultRange cannot be copied.

[schveiguy]

The ResultRange destroys itself if you make a copy. This makes for very unpleasant problems if you wrap by calling a function.

For instance:

struct S // simple example, real world is more complex
{
   this(ResultRange x) { r = x; } // destroying x kills the range
   ResultRange r;
   alias r this;
}

void main()
{
   auto conn = ... // create connection;
   auto r = S(conn.query("SELECT * FROM mytable"));
   foreach(row; r) // oops, it's empty!
   {
       writeln("got data: ", row);
   }
}

I'm not sure of a good way to fix this. Only thing I can think of is a reference counter.

[Abscissa]

Ouch, you're right. Seems that using struct dtors without refcounting should in general be regarded as a code smell?

[schveiguy]

Yes, it will be a very nice day indeed when D gets some sort of language-supported reference counting.

[schveiguy]

Note: I've worked around this in my project. I'm storing the range in a refcounted struct anyway, so I use std.algorithm.move to make sure the temporary copy doesn't kill the range.

A possible way to "fix" this without having to defensively wrap in a RefCounted, is to make the range uncopyable (@disable this(this)), and then provide a wrapper to allow it to be refcounted if you want to store it places. The suckiest part of this is that foreach(r; seq) makes a copy.

[schveiguy]

And further note (unrelated to this report), I have updated my code base to use version 1.1.0 (plus the PR you just merged), and it seems to work! I very much appreciate the handling of nulls better, my code got a LOT simpler because of that!

[Abscissa]

A possible way to "fix" this without having to defensively wrap in a RefCounted, is to make the range uncopyable (@disable this(this)), and then provide a wrapper to allow it to be refcounted if you want to store it places.

Hmm, yea, but uncopyable structs can be a pain to deal with, especially for langauge newcomers. I'm inclined to think thst just refcounting all the result ranges shouldn't be an unreasonable overhead for this.

Another possibility is to just get rid of the destructor. Looking at it now, all it does is ensure any remaining results get purged (which I guess partially answers your other question from over at #115 ). The downside then is that people (might?) have to make sure they manually purge any unused results. But maybe there's a better way I can handle that though: auto-purging before issuing any new commands. (Hmm...assuming I can do that, it would mean any existing incomplete resultsets would become invalidated and no longer usable - a condition they do safely detect on their own. Hmm, that may be workable, unless there was some specific reason I didn't do that...)

I'll have to give that some thought. What do you think?

And further note (unrelated to this report), I have updated my code base to use version 1.1.0 (plus the PR you just merged), and it seems to work! I very much appreciate the handling of nulls better, my code got a LOT simpler because of that!

Glad to hear it!

And yea, dealing with nulls used to be a real weak point with the lib. I think it was mostly leftover baggage from ancient versions of D that didn't have typeof(null), alias this, and really good implementations of Nullable/Variant. Also the fact that it used to take prepared stmt args by reference :/

[schveiguy]

What do you think?

I think that sounds perfect. It makes things actually pretty easy, and takes one more step out of the process. In my code, one ugly thing in my sql code is that I defensively put scopes everywhere, so I can be sure cleanup happens before I use the connection again. If using the connection again just cleans up everything, then I don't have to bother, making it simpler still!

[schveiguy]

One thing that's concerning though, the release of the prepared statement is a round trip to the server. It still technically should be done, but can't be done based on the range being destroyed any more, since the connection could be in use by another object.

What you may need to implement is a deferred cleanup of prepared statements, and when the connection restarts or is released from duty, it then processes all those.

[Abscissa]

One thing that's concerning though, the release of the prepared statement is a round trip to the server. It still technically should be done, but can't be done based on the range being destroyed any more, since the connection could be in use by another object.

Oh, so IIUC, you're thinking of this scenario (assuming we implemented auto-purging):

1. Prepared statement A is executed.
2. Some of prepared statement A's results are processed.
3. Prepared statement B looses all its references and is destructed.
4. B's dtor tells the server to release the statement (thus auto-purging A)
5. Program attempts to process the rest of A.

Seems unlikely to occur, but if it is at all possible, then yea you're right, that's a good point. That would need to be addressed, probably by a deferred prepared statement release.

[schveiguy]

Your understanding is pretty much correct, except I think actually this is not all that unlikely the way people write code.

e.g. something like this:

auto foo(Connection conn)
{
   auto a = conn.prepared("select * from table1 where id = ?");
   a.setArg(0, myId);
   auto seq = a.query();
   assert(!seq.empty);
   // read the relationship id
   auto id = seq.front.asAA["relationship_id"].get!int;
   // here, a is auto-purged by b's setup (which is OK, we were done with a).
   auto b = conn.prepared("select * from table2 where id = ?");
   b.setArg(0, id);
   // but here, a goes out of scope, and so a wants to send a message to the server
   // to release the prepared statement. That in turn auto-purges b before it can even be
   // used by the calling function.
   return b.query();
}

[Marenz]

Is this the same reason why I can't do this?

auto conn = new Connection(..);

auto ids = conn.query("SELECT id FROM example")
    .map!(a=>a[0].get!ulong).array;

// ids has length 0

[schveiguy]

@Marenz yes I believe so.

If you look at the implementation of map, you will see it accepts the range by value. However, it does not move the range into the map result, so once the function returns, the copy on map's stack is destroyed, and the result is purged.

My workaround in my code is to move the range into my wrapper, so it doesn't purge the data. But obviously you can't do this in Phobos, and the range in any case shouldn't be so brittle.

[Marenz]

A maybe more crude work-around could be to have a save function that returns a copy that is marked as "do not destroy"?

[mathias-baumann-sociomantic]

Expanding a bit on this problem, why don't you follow the same approach that https://dlang.org/phobos/std_container_array.html follows.

Basically, make the struct itself uncopyable, even disable the copy c'tor so you can't accidentally pass it to a non-ref function.
If one wants to use it as a range etc, offer opSlice[] which does return a copy-able range of that struct which is referencing the original non-copyable stack allocated struct.

Usage could look like this:

auto conn = new Connection(..);

auto ids = conn.query("SELECT id FROM example")[]
    .map!(a=>a[0].get!ulong).array;

[schveiguy]

Thanks @Abscissa! Look forward to simplifying my wrappers :)