Hi @igieon, in what scenarios would we face the need this close on connect to be executed? Have you faced a real situation? (I'm wondering e.g. why the old connection would have been left unclosed).

For example we have test for BA integration we modify Source database, than run our etl transformation and check destination db and then we want to drop destination db, but sometimes we cant drop in teardown because open connection and just form list open connection i see that connection come from dbfit. One solution is always before new connect call command close which is implemented. This is just quick fix which i think is usefull.

What about using |Close| command in the DbFit test page?

yes that is also option ;-). That i suggest but its error prone if someone forgot it.

I believe it's cleaner to have explicit |Close| in such cases. If you care about having connection closed earlier - it's good to have this visible and |Close| "speaks" better about this activity. If I see |Connect|... - it may not come to my mind that it's cleaning up the previous connection.

I think yes that is much better.

But then will be better just to add to the documentation ;-).

And probably for cleaner test to write |close| it will be good throw exception in case of new connect ?

But then will be better just to add to the documentation ;-).

Yes, it seems a good idea to add some guidelines for such scenarios.

And probably for cleaner test to write |close| it will be good throw exception in case of new connect ?

That's maybe not a bad option. I'm still not sure that we need to write extra code (even though it's just 1 line) in order to handle these cases. Some may not bother so much about connections being closed with some delay (by the garbage collector or when tests run ends) if it's not critical. We should think whether it makes sense to enforce a bit more discipline in these cases?

For us make sense as i write how i discover this is that we before test drop and recreate basic state of DB and then run test. And problem come as we run this tests in suite then we can't drop database. SO i think it will help someone other with same use-case to avoid this problem.

OK, let's implement that.

Are we going to break lots of other user's existing test assets?

Are we going to break lots of other user's existing test assets?

That's a good question and we don't know the answer. There might be existing tests which would get broken... (not quite disciplined in terms of releasing connections, yet they'd been working so far).

So what do we prefer?

• attempt to help development of cleaner tests in future (enforce explicit close when needed). Existing broken tests would need to be fixed
• keep the current state (pros: existing tests keep running; we don't complicate the code)
• as initially suggested - close the connection when trying to connect again

One thing I'm wondering about - even if we add some checks when trying to open a new connection, what about the following scenario (maybe even more common one):

1. Test db is created in the beginning of the test.
2. Some tests performed.
3. At the end of the test page / or tear down: drop the database

And here there is no second |Connect| it's just a single connection.

@igieon, how would you handle such scenario? Also how hard was it for you to find out that it was an open connection that prevented dropping the database?

In first it was not so hard to find what have open connection because when drop command failed i just added before drop one command to select all active connection with connected user and there is info who (ip) what is last command (so i seee exactly last select or commit).

And about drop db in tear down we have really oposit we drop db and recreate correct in setup phase, but i think that second will be problem.

And here there is no second |Connect| it's just a single connection.

In Flow Mode I think the connection will get closed by DbFit (because DatabaseTest is based upon SequenceFixture). But in Standalone Mode it will be left hanging about.

And here there is no second |Connect| it's just a single connection.

In Flow Mode I think the connection will get closed by DbFit (because DatabaseTest is based upon SequenceFixture). But in Standalone Mode it will be left hanging about.

Right. The natural place for tearing down resources (like dropping database) is at the end of the test page (e.g. via TearDown page). And for such cases tweaking the connect command (to raise exception, etc.) won't make any difference.

Here are few points which may lead us to some decision:

1. Currently it's expected that in FlowMode DbFit automatically takes care about closing connection by the end of the test. It's not explicitly defined what should be done if multiple connections have been opened - but expecting DbFit to close of them all is not so unreasonable.
2. It may be a desired effect to keep the 1st connection open after 2nd one is opened. E.g. - to test that two concurrent sessions may access a shared resource, or to test that operation from a 2nd session would be rejected (if 1st one has locked the resource).
3. We may add ability to have and switch back between multiple concurrent connections in one and the same test.

There might be existing tests which rely on the current behaviour as per 1. For 2 - I'm not quite sure, it should be rare I guess. Should we maintain a list of all connections and closeAll at the end?

Yes i am for option 1. That mean at the end all connections is closed (that was our expectation when we use multiple connections. So i can modify patch that close at the end all connections. So that will do exactly what we need.

Option 2. For me doesnt make sense in our case we do it open 1 connection make some insert, run tested program, open second connection to verify results adn we expected that all connection is closed. Probably that every one goest with that expectation.

If we allow multiple connections to be created but then don't provide a way to close each of them (as is the case now) then we're relying on the DB server to time them out and, probably, roll back their transactions if appropriate, or perhaps require users to execute some DB code or utility to clean them up. I.e. there's something missing in our connections management.

In the long term perhaps a variant of each of the |Connect| methods could allow users to associate a handle/name for each connection opened and allow us, via a new fixture, to switch the active connection.

But until then, or if ever, perhaps a generic method for closing all DbFit-initiated connections would be good. Perhaps establishing a closeAll() method would be the right thing and this could be called at the end of a Flow-Mode test page. If we allowed a |Close|All| fixture variant, too, then perhaps we could at least provide the same ability to clean-up connections, manually, in Standalone-Mode.

In addition, perhaps we can limit the breaking of existing users' tests by allowing any change like this to be controlled via something like a new |Set Option|Close All| option (for Flow-Mode only).

If we assume that nobody indeed relies on case 2 - then the code change which we already have in this PR (close the old connection before opening a new one) should be enough I guess.

If we assume that nobody indeed relies on case 2 - then the code change which we already have in this PR (close the old connection before opening a new one) should be enough I guess.

Should we look to do something for Standalone-mode too in this PR?

What part is standalone mode ?

Should we look to do something for Standalone-mode too in this PR?

Standalone is more or less manual mode. Maybe we don't need to do anything about it.

What part is standalone mode ?

It's where we don't start tests/pages with one of the fixtures that sub-class DatabaseTest, such as OracleTest, but instead uses DatabaseEnvironment.

Standalone is more or less manual mode. Maybe we don't need to do anything about it.

May be. But if not then we're still able to create connections in this mode that we can't close or clear up.

May be some users are holding connections open across different test pages, in a suite, and so we'd end up breaking other tests if we implemented something. It would be good to review this in #487.

May be. But if not then we're still able to create connections in this mode that we can't close or clear up.

That should be possible e.g. via:

|DatabaseEnvironment|
|close|

Yet, a reasonable question is why we're not doing that in DbFit own tests...

May be some users are holding connections open across different test pages, in a suite, and so we'd end up breaking other tests if we implemented something.

Yes, that's a real case: https://groups.google.com/d/msg/dbfit/l2m1_c4BDkQ/T23VwKXBNEkJ, https://groups.google.com/d/msg/dbfit/AQu_qrlUBSE/3GZVvnvklAYJ

That should be possible e.g. via:

|DatabaseEnvironment| |close|

I mean if someone were to create a second connection without closing the first. We'll currently end up with a rogue one that can't be closed (until the currently proposed patch is applied).

Yet, a reasonable question is why we're not doing that in DbFit own tests...

Not quite sure what you mean. Can you explain differently?

I mean if someone were to create a second connection without closing the first. We'll currently end up with a rogue one that can't be closed (until the currently proposed patch is applied).

Yes, in standalone mode if you want to have the connection closed, you should invoke |DatabaseEnvironment|close|. It's not closed automatically closed nor at end of test nor when new connection is opened. (Well, after the current patch we may end up having it closed in the rare case when close is invoked without specific database type specified).

Yet, a reasonable question is why we're not doing that in DbFit own tests...

Not quite sure what you mean. Can you explain differently?

E.g. here in TearDown we're doing rollback but the connection is left open:
https://github.com/dbfit/dbfit/blob/master/FitNesseRoot/DbFit/AcceptanceTests/JavaTests/OracleTests/StandaloneFixtures/TearDown/content.txt

So once this patch is applied we'll have the same handling for establishing a second connection (the first one will get closed) in both modes.

E.g. here in TearDown we're doing rollback but the connection is left open:

Right. I see. I didn't realise we weren't closing them down. I can log an issue for this just to tidy this up.

So once this patch is applied we'll have the same handling for establishing a second connection (the first one will get closed) in both modes.

In standalone mode (most) often the database type (Oracle, MySql, etc.) is explicitly specified:

|DatabaseEnvironment|Oracle|
|connect|.....|

I think in such case the patch won't have impact since a new environment instance is created.

Right. I see. I didn't realise we weren't closing them down. I can log an issue for this just to tidy this up.

OK.

I think in such case the patch won't have impact since a new environment instance is created.

Ah, yes.

In that case should be look to close any existing connection that's open and available the previous environment via DbEnvironmentFactory.getDefaultEnvironment()?

In that case should be look to close any existing connection that's open and available the previous environment via DbEnvironmentFactory.getDefaultEnvironment()?

I would say we shouldn't bother. In standalone mode we don't explicitly close connections - that's how it used to be for single connection too. If timely closing is important - there is a |close| command. If not - the garbage collector or the end of test run end (java process exit) will clean it up.

In FlowMode we rollback and close not later than the end of the test.

If not - the garbage collector or the end of test run end (java process exit) will clean it up.

Ok. Although I still think one could end up with DB locks if the DB session doesn't always get killed because the parent Java process has ended. I've certainly seen this across different DB types in the past. I guess we can revisit this all on the concurrent-connections feature work.

Opening a new connection without explicit |Close| of the old one is not encouraged (unless there is some specific reason to do the contrary). Even in Flow Mode. As discussed earlier - it's good to update the docs with a note about that.

SO how we change this patche set. DO we only close or we add additional option that throw exception if you open connection without closing previous one ?

Thank you @igieon. Let's keep it as it is now. Just could you please add a note to the docs (the manual is within dbfit/website/_includes/manual) - for Connect and ConnectUsingFile, that it's advised to do a |Close| before opening a 2nd connection.

Like this ?

Its ok now ?

Its ok now ?

Thanks! Looks OK, I have just one cosmetic remark about the blank line. Also could you please squash the two commits into one.

Fixed and squashed

I just did a test run of the docs using https://jekyllrb.com/. Seems my suggestion for removing the blank line at the end was not a good idea (it's messing up the rendering of the next section).

@igieon, could you please update with blank line at the end of these files. You may just checkout my version which I posted to dbfit/topic/close-connection-on-second-open (I also updated a bit the commit comment).

Ok used your commit from https://github.com/dbfit/dbfit/tree/topic/close-connection-on-second-open

Great. I'm merging the PR. @igieon, thank you very much for your contribution!