On 03/07/2018 04:53 PM, bauss wrote:
> 
> I can't seem to reproduce it now, but I'll keep an eye for it and see if it still happens, but I think the problem is when you return the connection from a function.
> 
> I had similar issues returning a raw connection created.

By any chance, are you ever storing a Connection or a ResultRange anywhere? I don't mean as a function-local variable or a a function parameter: I mean like as a class/struct member or as a global? (Well, not that D really has true globals, but a "global" at the module-level.)

If you keep around a Connection (or a ResultRange, which has a reference to its own Connection) past the end of the vibe.d task that was using it, then that can definitely cause this kind of problem.

(AIUI, vibe.d wants to make the connections you obtain from a pool be Scoped. That would help prevent users from accidentally storing something they shouldn't and running into this issue.)

On Thursday, 8 March 2018 at 07:03:15 UTC, Nick Sabalausky (Abscissa) wrote:
> On 03/07/2018 04:53 PM, bauss wrote:
>> 
>> I can't seem to reproduce it now, but I'll keep an eye for it and see if it still happens, but I think the problem is when you return the connection from a function.
>> 
>> I had similar issues returning a raw connection created.
>
> By any chance, are you ever storing a Connection or a ResultRange anywhere? I don't mean as a function-local variable or a a function parameter: I mean like as a class/struct member or as a global? (Well, not that D really has true globals, but a "global" at the module-level.)
>
> If you keep around a Connection (or a ResultRange, which has a reference to its own Connection) past the end of the vibe.d task that was using it, then that can definitely cause this kind of problem.
>
> (AIUI, vibe.d wants to make the connections you obtain from a pool be Scoped. That would help prevent users from accidentally storing something they shouldn't and running into this issue.)

Yeah I stores the result range which I assume was the issue, but I'm returning results as arrays now so it probably solved it.

That behavior should be documented though, I don't recall reading that anywhere and it's kind of a gotcha

On 03/08/2018 02:14 AM, Bauss wrote:
>>
>> By any chance, are you ever storing a Connection or a ResultRange anywhere? I don't mean as a function-local variable or a a function parameter: I mean like as a class/struct member or as a global? (Well, not that D really has true globals, but a "global" at the module-level.)
>>
>> If you keep around a Connection (or a ResultRange, which has a reference to its own Connection) past the end of the vibe.d task that was using it, then that can definitely cause this kind of problem.
>>
>> (AIUI, vibe.d wants to make the connections you obtain from a pool be Scoped. That would help prevent users from accidentally storing something they shouldn't and running into this issue.)
> 
> Yeah I stores the result range which I assume was the issue, but I'm returning results as arrays now so it probably solved it.
> 
> That behavior should be documented though, I don't recall reading that anywhere and it's kind of a gotcha
> 

Agreed. Please file a ticket for this so I don't forget.

On Thursday, 8 March 2018 at 06:49:52 UTC, Nick Sabalausky (Abscissa) wrote:
> On 03/07/2018 02:32 PM, bauss wrote:
>> 
>> Wait why has it been updated to array() ? So it's not a real range anymore? Or was it always represented as an array behind the scenes?
>> 
>> I just feel like allocating it into an additional array is a waste of memory? But if it was always like that I guess it doesn't matter.
>> 
>
> query() returns an input range. You can only access one element at a time (as its read from the network) and you don't know how many there are ahead of time, BUT it avoids allocating a whole array to store everything.
>
> In addition to query(), there used to also be a querySet(). The querySet() would allocate an array and read ALL the results into it so you could get random-access. But that's exactly what you already get when you call array() on an input range (such as the input range returned by query), so querySet was deemed redundant and eliminated.
>
> So if you had code that *did* need an array allocated to store all the results, then "querySet()" has been replaced with "query().array". But like you said, if you don't really need the array, then there's no need to call array() and waste the memory.
>
>
>> However idk what I changed, but the issue stopped for me.
>> 
>> However I still have this issue:
>> 
>> https://github.com/mysql-d/mysql-native/issues/153
>> 
>> (Currently trying to see if I can make a minimal example, but it's kinda hard to make a minimal example since it's from my Diamond MVC (vibe.d) library and it isn't used until deep nesting of the application.
>> 
>> Anyway before I report anything else I could easily be doing something wrong. There hasn't exactly been any good examples on how to use it with vibe.d so it has pretty much been a trial and error thing for me.
>
> Using mysql-native with vibe.d isn't any different from using it without vibe.d.
>
> It's recommended to use MySQLPool to make a Connection rather than doing "new Connection" directly simply because connecting is faster that way (though "new Connection" will still work).
>
> But aside from that, there is absolutely nothing different about mysql-native whether you're using vibe.d or not.
>
>
>> So basically I keep an associative array of connection pools based on connection strings like below:
>> 
>> private static __gshared MySQLPool[string] _pools;
>> 
>> And then I retrieve a connection with the function below.
>> 
>> Perhaps I'm not supposed to make a new pool every time, but there is someway to retrieve a pool already? Maybe that's what I'm doing wrong?
>> 
>> private static shared globalPoolLock = new Object;
>> 
>> private Connection getMySqlConnection(string connectionString)
>> {
>>    auto pool = _pools.get(connectionString, null);
>> 
>>    if (!pool)
>>    {
>>      synchronized (globalPoolLock)
>>      {
>>        pool = new MySQLPool(connectionString);
>> 
>>        _pools[connectionString] = pool;
>>      }
>>    }
>> 
>>    return pool.lockConnection();
>> }
>> 
>> After I retrieve the connection then it's basically like the code I showed you, but that seem to be correct, yes?
>
> Does your application need to support multiple connection strings while it's running? That's pretty rare unless you're making something like phpMyAdmin (and even then, I'd probably do it a little differently). Normally you'd just make one connection pool:
>
> MySQLPool pool;
>
> Then "new" that once with your connection string when you start up, and you're good.
>
> I guess I can imagine some potential use-cases that get more complicated than that, but that's really up to your own project's needs.
>
> > However I still have this issue:
> >
> > https://github.com/mysql-d/mysql-native/issues/153
> >
> > (Currently trying to see if I can make a minimal example, but
> it's kinda
> > hard to make a minimal example since it's from my Diamond MVC
> (vibe.d)
> > library and it isn't used until deep nesting of the
> application.
>
> I'm only guessing here, but I wonder if that might be because you seem to be trying to share pools and connections across threads. I don't know whether vibe is designed to share TCP connections across threads or not. I'd say, try ripping out all that shared/__gshared/synchronized stuff and see how that works.

But if you can't store the pools anywhere, how are you supposed to use them with vibe.d?

Creating a new pool for every thread seems expensive and dosn't that defeat the purpose of using pools in the first place?

I mean a 1000 people could connect to a website and potentially that could create a thousand threads (It probably won't, BUT it could.) and thus it can't be afforded to create a pool per request.

I mean it's kind of a corner case, but it's a common corner case for big applications.

I don't create any explicit threads.

But like I said, it seems to work after I stopped returning the connection and just the pool.

so I think the problem wasn't the pool, but the connection itself.

At least I can't reproduce it and tried with hundreds of queries at once and before I could reproduce with less than 10.

And yes I need support for multiple connection strings. It's not an application, it's a library for writing enterprise websites/webapis and thus it should be scalable, since most enterprise solutions uses multiple databases.

On 03/08/2018 06:57 AM, bauss wrote:
> 
> But if you can't store the pools anywhere, how are you supposed to use them with vibe.d?

You can store the pools wherever you need to, just don't hold onto a Connection past the end of a vibe task.

> Creating a new pool for every thread seems expensive and dosn't that defeat the purpose of using pools in the first place?
> 
> I mean a 1000 people could connect to a website and potentially that could create a thousand threads (It probably won't, BUT it could.) and thus it can't be afforded to create a pool per request.

This is more a question for vibe's Sonke, to be honest.

What I do know is that vibe's basic task distribution typically involves one task/request per *fiber*, not per thread. And that vibe is based heavily around handling multiple simultaneous requests/tasks/fibers within a single thread.

Beyond that, I have no idea how vibe deals with threads, hence my suggestion that if you're having trouble, it might be worth seeing whether going totally thread-local with everything might help. Just a thought. At the very last it might help narrow it down.


> I mean it's kind of a corner case, but it's a common corner case for big applications.
> 
> I don't create any explicit threads.
> 
> But like I said, it seems to work after I stopped returning the connection and just the pool.
> 
> so I think the problem wasn't the pool, but the connection itself.
> 
> At least I can't reproduce it and tried with hundreds of queries at once and before I could reproduce with less than 10.
> 

I admit I'm unclear at this point which problem you're referring to, and whether or not you're saying it's fixed now. (Pardon my confusion - been a long day.)

If you mean issue #153, then I wonder if that might be the same cause as #170 (Same symptoms at least: "Acquiring waiter that is already in use"):

#170: https://github.com/mysql-d/mysql-native/issues/170

Sonke figured out just a few hours ago, and I confirmed, that #170 is caused by a bug in MySQLPool.lockConnection introduced in v2.1.0. I plan to have that fixed with a new release today.

That bug turned out to be caused by MySQLPool.lockConnection allowing the LockedConnection!Connection it received from vibe to degrade down to a plain Connection (because of alias this allowing implicit conversion). So MySQLPool.lockConnection would then return the *raw* Connection instead, thus allowing LockedConnection to go out of scope and reclaim the connection as soon as MySQLPool.lockConnection returned.

Sonke's been putting thought into how to adjust vibe's LockedConnection so this can't happen by accident. Maybe your lib fell prey to the same accident mine did?


> And yes I need support for multiple connection strings. It's not an application, it's a library for writing enterprise websites/webapis and thus it should be scalable, since most enterprise solutions uses multiple databases.

Ah, I see. Fair enough.

On Thursday, 8 March 2018 at 13:33:32 UTC, Nick Sabalausky (Abscissa) wrote:
> Sonke figured out just a few hours ago, and I confirmed, that #170 is caused by a bug in MySQLPool.lockConnection introduced in v2.1.0. I plan to have that fixed with a new release today.
>
> That bug turned out to be caused by MySQLPool.lockConnection allowing the LockedConnection!Connection it received from vibe to degrade down to a plain Connection (because of alias this allowing implicit conversion). So MySQLPool.lockConnection would then return the *raw* Connection instead, thus allowing LockedConnection to go out of scope and reclaim the connection as soon as MySQLPool.lockConnection returned.
>
> Sonke's been putting thought into how to adjust vibe's LockedConnection so this can't happen by accident. Maybe your lib fell prey to the same accident mine did?
>
>
>> And yes I need support for multiple connection strings. It's not an application, it's a library for writing enterprise websites/webapis and thus it should be scalable, since most enterprise solutions uses multiple databases.
>
> Ah, I see. Fair enough.

That's possible. Looking forward to the release, then I'll test it out and see what happens.

On 3/8/18 6:57 AM, bauss wrote:
> On Thursday, 8 March 2018 at 06:49:52 UTC, Nick Sabalausky (Abscissa) wrote:
>> On 03/07/2018 02:32 PM, bauss wrote:
>>>
>>> Wait why has it been updated to array() ? So it's not a real range anymore? Or was it always represented as an array behind the scenes?
>>>
>>> I just feel like allocating it into an additional array is a waste of memory? But if it was always like that I guess it doesn't matter.
>>>
>>
>> query() returns an input range. You can only access one element at a time (as its read from the network) and you don't know how many there are ahead of time, BUT it avoids allocating a whole array to store everything.
>>
>> In addition to query(), there used to also be a querySet(). The querySet() would allocate an array and read ALL the results into it so you could get random-access. But that's exactly what you already get when you call array() on an input range (such as the input range returned by query), so querySet was deemed redundant and eliminated.
>>
>> So if you had code that *did* need an array allocated to store all the results, then "querySet()" has been replaced with "query().array". But like you said, if you don't really need the array, then there's no need to call array() and waste the memory.
>>
>>
>>> However idk what I changed, but the issue stopped for me.
>>>
>>> However I still have this issue:
>>>
>>> https://github.com/mysql-d/mysql-native/issues/153
>>>
>>> (Currently trying to see if I can make a minimal example, but it's kinda hard to make a minimal example since it's from my Diamond MVC (vibe.d) library and it isn't used until deep nesting of the application.
>>>
>>> Anyway before I report anything else I could easily be doing something wrong. There hasn't exactly been any good examples on how to use it with vibe.d so it has pretty much been a trial and error thing for me.
>>
>> Using mysql-native with vibe.d isn't any different from using it without vibe.d.
>>
>> It's recommended to use MySQLPool to make a Connection rather than doing "new Connection" directly simply because connecting is faster that way (though "new Connection" will still work).
>>
>> But aside from that, there is absolutely nothing different about mysql-native whether you're using vibe.d or not.
>>
>>
>>> So basically I keep an associative array of connection pools based on connection strings like below:
>>>
>>> private static __gshared MySQLPool[string] _pools;
>>>
>>> And then I retrieve a connection with the function below.
>>>
>>> Perhaps I'm not supposed to make a new pool every time, but there is someway to retrieve a pool already? Maybe that's what I'm doing wrong?
>>>
>>> private static shared globalPoolLock = new Object;
>>>
>>> private Connection getMySqlConnection(string connectionString)
>>> {
>>>    auto pool = _pools.get(connectionString, null);
>>>
>>>    if (!pool)
>>>    {
>>>      synchronized (globalPoolLock)
>>>      {
>>>        pool = new MySQLPool(connectionString);
>>>
>>>        _pools[connectionString] = pool;
>>>      }
>>>    }
>>>
>>>    return pool.lockConnection();
>>> }
>>>
>>> After I retrieve the connection then it's basically like the code I showed you, but that seem to be correct, yes?
>>
>> Does your application need to support multiple connection strings while it's running? That's pretty rare unless you're making something like phpMyAdmin (and even then, I'd probably do it a little differently). Normally you'd just make one connection pool:
>>
>> MySQLPool pool;
>>
>> Then "new" that once with your connection string when you start up, and you're good.
>>
>> I guess I can imagine some potential use-cases that get more complicated than that, but that's really up to your own project's needs.
>>
>> > However I still have this issue:
>> >
>> > https://github.com/mysql-d/mysql-native/issues/153
>> >
>> > (Currently trying to see if I can make a minimal example, but
>> it's kinda
>> > hard to make a minimal example since it's from my Diamond MVC
>> (vibe.d)
>> > library and it isn't used until deep nesting of the
>> application.
>>
>> I'm only guessing here, but I wonder if that might be because you seem to be trying to share pools and connections across threads. I don't know whether vibe is designed to share TCP connections across threads or not. I'd say, try ripping out all that shared/__gshared/synchronized stuff and see how that works.
> 
> But if you can't store the pools anywhere, how are you supposed to use them with vibe.d?
> 
> Creating a new pool for every thread seems expensive and dosn't that defeat the purpose of using pools in the first place?
> 
> I mean a 1000 people could connect to a website and potentially that could create a thousand threads (It probably won't, BUT it could.) and thus it can't be afforded to create a pool per request.

Well, vibe.d shouldn't be doing that. It creates fibers per request, not threads, no?

I would expect no more than one thread per core to be created, and you can afford that many pools (I'd create them at program/thread startup in any case).

The point of a pool is to avoid some costly setup. In my case, I'm not even closing the connection because I feel the "cost" of allocating a connection from the heap isn't worth worrying about. But I also limit the pool so it's only going to allow X concurrent Db connections per thread.

-Steve

On Thursday, 8 March 2018 at 15:09:07 UTC, Steven Schveighoffer wrote:
> The point of a pool is to avoid some costly setup. In my case, I'm not even closing the connection because I feel the "cost" of allocating a connection from the heap isn't worth worrying about. But I also limit the pool so it's only going to allow X concurrent Db connections per thread.
>
> -Steve

If you limit the amount of concurrent db connections will the task wait until a connection can be acquired or will it throw an exception?

On 3/8/18 11:14 AM, bauss wrote:
> On Thursday, 8 March 2018 at 15:09:07 UTC, Steven Schveighoffer wrote:
>> The point of a pool is to avoid some costly setup. In my case, I'm not even closing the connection because I feel the "cost" of allocating a connection from the heap isn't worth worrying about. But I also limit the pool so it's only going to allow X concurrent Db connections per thread.
>>
> 
> If you limit the amount of concurrent db connections will the task wait until a connection can be acquired or will it throw an exception?

It uses a semaphore with a max count, so it would just pause the fiber.

https://github.com/vibe-d/vibe.d/blob/master/core/vibe/core/connectionpool.d#L55

-Steve

On Thursday, 8 March 2018 at 16:24:22 UTC, Steven Schveighoffer wrote:
> On 3/8/18 11:14 AM, bauss wrote:
>> On Thursday, 8 March 2018 at 15:09:07 UTC, Steven Schveighoffer wrote:
>>> The point of a pool is to avoid some costly setup. In my case, I'm not even closing the connection because I feel the "cost" of allocating a connection from the heap isn't worth worrying about. But I also limit the pool so it's only going to allow X concurrent Db connections per thread.
>>>
>> 
>> If you limit the amount of concurrent db connections will the task wait until a connection can be acquired or will it throw an exception?
>
> It uses a semaphore with a max count, so it would just pause the fiber.
>
> https://github.com/vibe-d/vibe.d/blob/master/core/vibe/core/connectionpool.d#L55
>
> -Steve

Great