Add cleanup function.

[schveiguy]

Vibe-core now includes a mechanism to clean up all unused connection pool connections so the event driver can be properly destroyed. The way mysql-native's pool wraps vibe's ConnectionPool makes it difficult to reproduce the same functionality AND provide access to the cleanup function. In addition, this makes sure it does the cleanup properly.

In my code, if I don't have this function, I get "Leaking eventcore driver" messages.

I tried to make sure this properly shows up in the docs, we'll see what happens. I also tried to ensure this function only gets defined for vibe-core that supports it (requires 0.8.6). We will see if the CI accepts that.

[Abscissa]

Vibe-core now includes a mechanism to clean up all unused connection pool connections so the event driver can be properly destroyed. The way mysql-native's pool wraps vibe's ConnectionPool makes it difficult to reproduce the same functionality AND provide access to the cleanup function. In addition, this makes sure it does the cleanup properly.

Hmm, this is new to me. Any recommendation on how mysqln's pool could be improved in this regard, or what specifically the problem is?

In my code, if I don't have this function, I get "Leaking eventcore driver" messages.

Ahh, mysqln's travis tests have been getting that, too. I wonder if there's any way this could be used to fix that? I'd guess maybe not, at least not without too much trouble, just because of the way D testrunners work (no real concept of "last line of code before exiting" or "on exit hook" that I'm aware of).

I tried to make sure this properly shows up in the docs, we'll see what happens.

It should be easy enough to build the docs yourself, just run the "./build-docs" script. If that doesn't work right out-of-the box go ahead and file an issue.

In any case, if the function only exists under certain conditions, that should probably be explained in the function's docs.

I also tried to ensure this function only gets defined for vibe-core that supports it (requires 0.8.6). We will see if the CI accepts that.

The 0.8.6 is vibe-d:core. Package vibe-core is 1.0.0 and up. I don't know if that means the function exists in all versions of vibe-core, but if so, then making it conditional may not be needed because mysqln master is now vibe-core-only (as of yesterday), no more vibe-d:core (it would be far too problematic to try to support both, and luckily vibe-d:core seems end-of-life now anyway).

Bear in mind, since I switched it to vibe-core, travis is ATM currently only testing against vibe-core 1.7.0 (the latest non-beta). I plan to add tests for earlier versions very shortly, and see how far back can realistically be supported (but without going so far back as vibe-d:core).

[Abscissa]

If this function only exists under certain conditions, that should probably be explained in its doc comments here.

[schveiguy]

Updated

[schveiguy]

Hmm, this is new to me. Any recommendation on how mysqln's pool could be improved in this regard, or what specifically the problem is?

Well, the specific problem is that MySqlPool does a lot more than just wrapping a ConnectionPool, it has mechanisms to call functions every time a connection is opened, handles all the prepared statements that need to be released, etc. I'm not sure all of these could be called by user code anyway. In fact the note saying it's a "simple wrapper" and you can just use ConnectionPool directly instead might be outdated. I don't really know.

[schveiguy]

The 0.8.6 is vibe-d:core. Package vibe-core is 1.0.0 and up.

Both vibe-d:core and vibe-core (I think version 1.7.0+) have the function available I think. What I meant is that if you link this against earlier versions of the library (maybe for other reasons), this function would not be available.

[schveiguy]

Ahh, mysqln's travis tests have been getting that, too. I wonder if there's any way this could be used to fix that? I'd guess maybe not, at least not without too much trouble, just because of the way D testrunners work (no real concept of "last line of code before exiting" or "on exit hook" that I'm aware of).

This is callable at any time. All it does is close any unused connections that have not been closed (i.e. they are in the pool but nobody has any references to them). So I'd say go ahead and put a scope(exit) in the tests. Where do I do that?

[Abscissa]

Well, the specific problem is that MySqlPool does a lot more than just wrapping a ConnectionPool, it has mechanisms to call functions every time a connection is opened, handles all the prepared statements that need to be released, etc. I'm not sure all of these could be called by user code anyway.

Hmm, I guess I’m just unclear/confused about what you’re talking about with this:

The way mysql-native's pool wraps vibe's ConnectionPool makes it difficult to reproduce the same functionality AND provide access to the cleanup function.

I think I’m just having a bit of a brain fart moment on this. IIUC: You’re saying there’s some new functionality in vibe’s connection pool that (due to the design of mysql-native’s connection pool) is difficult to use/expose in mysql-native’s connection pool. And therefore, the next best thing that can be done is simply to add this removeUnusedConnections() function as you’re doing in this PR. Is that correct?

In fact the note saying it's a "simple wrapper" and you can just use ConnectionPool directly instead might be outdated. I don't really know.

You may be right about that…

Both vibe-d:core and vibe-core (I think version 1.7.0+) have the function available I think. What I meant is that if you link this against earlier versions of the library (maybe for other reasons), this function would not be available.

Ah, ok, so there are older versions of vibe-core that lack it. That’s fine then. Maybe just one more adjustment to the docs then about how removeUnusedConnections only exists on newer vibe-core’s that support the removeUnused feature on connection pools. Aside from that, this looks good to me, and travis seems happy with it too, which is always good ;)

Ahh, mysqln's travis tests have been getting that, too. I wonder if there's any way this could be used to fix that?

This is callable at any time. All it does is close any unused connections that have not been closed (i.e. they are in the pool but nobody has any references to them). So I'd say go ahead and put a scope(exit) in the tests. Where do I do that?

Honestly, I wouldn’t worry too much about it. That would just mean the tests would constantly be re-creating new connections, which would just slow the tests down, and travis is already taking like 3-4 minutes on most of these jobs. If there were a trivial way to just to it once after all the tests were complete, then that’d be great, but as things stand, I don’t it’s worth it just to silence a few benign warnings at the end of the tests.

[schveiguy]

Hmm, I guess I’m just unclear/confused about what you’re talking about with this:

The way mysql-native's pool wraps vibe's ConnectionPool makes it difficult to reproduce the same functionality AND provide access to the cleanup function.

I think I’m just having a bit of a brain fart moment on this. IIUC: You’re saying there’s some new functionality in vibe’s connection pool that (due to the design of mysql-native’s connection pool) is difficult to use/expose in mysql-native’s connection pool. And therefore, the next best thing that can be done is simply to add this removeUnusedConnections() function as you’re doing in this PR. Is that correct?

Oh, yeah, definitely it's not possible to call the functionality because the ConnectionPool member is private! What I meant by the above comment is that if I wanted to simply use ConnectionPool instead of MySQLPool, just to get at the new functionality, then I lose out on all the benefits that MySQLPool has. I'm actually not sure if it's appropriate to use a ConnectionPool directly now anyway. I'm not sure how all those auto registration/etc functions work, or if they are required to make the new Prepared stuff work.

[schveiguy]

Maybe just one more adjustment to the docs then about how removeUnusedConnections only exists on newer vibe-core’s that support the removeUnused feature on connection pools.

Done

[Abscissa]

Oh, yeah, definitely it's not possible to call the functionality because the ConnectionPool member is private! What I meant by the above comment is that if I wanted to simply use ConnectionPool instead of MySQLPool, just to get at the new functionality, then I lose out on all the benefits that MySQLPool has.

Ah, ok, I see.

I'm actually not sure if it's appropriate to use a ConnectionPool directly now anyway. I'm not sure how all those auto registration/etc functions work, or if they are required to make the new Prepared stuff work.

Yea, I'd have to go digging around there again to be certain exactly what would happen, but it's definitely not a recommended approach, especially since the prepared statements are now pretty much the default and get used behind-the-scenes (rather than something you have to go out of your way to deliberately use, which is part of the whole intent).

So this all looks good now. The second commit is just comments and whitespace, so I think we can safely skip waiting for travis and trust its initial OK.