DatabaseTransport should not close a connection

[changlexc]

The DatabaseTransport accept a db conn, but it closed by DatabaseTransport finally. I don't think it's a good design because the conn may have other usage outside the DatabaseTransport. The conn lifecycle should be maintain by its creator but not closed by DatabaseTransport.

The caused lot of ansible module can not work, because after DatabaseTransport, we still use the conn to retrieve the job_log or use as another DatabaseTransport.

[kadler]

This is a deliberate design choice. The DatabaseTransport is meant to own the connection and is not meant be used outside of the transport.

[changlexc]

I don't think it's a good design since the outside conn will be closed imperceptibly. This breaks the existed behavior of lots customer's code.

[jkyeung]

I don't think it's a good design since the outside conn will be closed imperceptibly. This breaks the existed behavior of lots customer's code.

You're saying you already had lots of code using DatabaseTransport that used to work? Under which version of itoolkit? Can you just keep using that version? If not, why not?

If you mean it doesn't work the way you would like, so you can't write new code the way you would like, you have a couple of options. One is to fork this project and fix it to your liking. It may be easier to do this than to persuade Kevin to change the design.

To me, a much simpler option is to just use the package as it is and adapt your own code to fit.

There are many things you can do to adapt. The most straightforward is probably to just make sure to always create a new connection that is meant solely for the transport. The docstring for the DatabaseTransport class already contains examples for this:

>>> from itoolkit.transport import DatabaseTransport
>>> import pyodbc
>>> transport = DatabaseTransport(pyodbc.connect('DSN=*LOCAL'))

or

>>> from itoolkit.transport import DatabaseTransport
>>> import ibm_db_dbi
>>> transport = DatabaseTransport(ibm_db_dbi.connect())

If you do it that way, you don't even have a separately usable reference to the connection.

You also mention that

after DatabaseTransport, we still use the conn to retrieve the job_log or use as another DatabaseTransport.

OK, so another way you could adapt is to keep the DatabaseTransport open for as long as you need the connection. Why do you need to create a new DatabaseTransport on the same connection anyway? Why can't you just keep using the one you already have?

[changlexc]

If you look at Ansible for IBMi(https://github.com/IBM/ansible-for-i), we already has the framework to keep a conn during the Ansible lifecycle but not the DatabaseTransport lifecycle. A typical case is we need use the conn to prompt the authority by calling qsygetph before and then do DatabaseTransport then finally change back the authority, the conn should the same one in order to run in the same job. But now, the conn got closed after DatabaseTransport. Another case is we first got the job information and then run DatabaseTransport and then retrieve the joblog, it also needs the same conn. But after the DatabaseTransport, the conn got closed, so we can not retrieve joblog by that closed conn. So many cases here. And lot of our Ansible on ibmi customer meet the issue when they upgrade the python39 and python39-itoolkit, I check the version, the close conn feature are in 1.7, so under 1.6.0, it doesn't have this error.

We can not use a fixed version(e.g. 1.6.2) under our project but not use the site-packs, because Ansible is not a rpm installed on IBMi. It running as a client of ssh by sending some generated code to target IBMi to run.

I think a better way may be at least give a compatible option with existing code, e.g. like the DatabaseTransport could have a parm to choice to close the conn or not.

[changlexc]

The source code I mentioned is https://github.com/IBM/ansible-for-i/blob/devel/plugins/module_utils/ibmi/ibmi_module.py

[jkyeung]

The source code I mentioned is https://github.com/IBM/ansible-for-i/blob/devel/plugins/module_utils/ibmi/ibmi_module.py

After looking briefly at that code, I don't see why you can't refactor it so that you don't keep creating new DatabaseTransport objects. You should be able to just create one (right after you create a connection) and reuse it.

[changlexc]

The fact is the conn closed after DatabaseTransport was called, we have other modules use the ibmi_module.py in plugins.

For example, in https://github.com/IBM/ansible-for-i/blob/devel/plugins/modules/ibmi_cl_command.py,
line 220 rc, out, err, job_log = ibmi_module.itoolkit_run_command_once(command)
line 221 job_name_info = ibmi_module.get_current_job_name()
After line 220 run, the conn closed in 221

Even in the
def itoolkit_run_command_once(self, command):
'''This method equals to itoolkit_run_command and itoolkit_get_job_log'''
rc, out, error = self.itoolkit_run_command(command)
job_log = self.get_job_log('*', self.startd)
return rc, out, error, job_log

Line job_log = self.get_job_log('*', self.startd) got exception " ibm_db_dbi::ProgrammingError: Cursor cannot be returned; connection is no longer active."

[changlexc]

Or do you mean I should use itransport = DatabaseTransport(ibm_db_dbi.connect()) instead of itransport = DatabaseTransport(self.conn)

So the DatabaseTransport just closes the ibm_db_dbi.connect() but not self.conn ?

[changlexc]

I tried using the new conn in DatabaseTransport, unfortunately the DatabaseTansport conn job is 'jobname': 'QSQSRVR', 'jobuser': 'QUSER', 'jobnbr': '126387', but the original conn's job is 126115/QUSER/QSQSRVR. This is not what we expected. We use a single conn is intend to make all the executions happened a same job, so that we can get more information about that job to monitor the joblog etc..., these information are very important for us.

[kadler]

Did some digging and this behavior was changed in #66 for issue #64.

The nice thing is that it is all within your ability to override this behavior by subclassing the transport: https://github.com/IBM/python-itoolkit/blob/main/src/itoolkit/transport/base.py#L72-L75

You can add something like this to your code and don't even need to depend on any changes here:

class UnclosingDatabaseTransport(DatabaseTransport):
    def _close(self):
        """Don't close connection, we'll manage it ourselves"""
        pass

[jkyeung]

Good thing Kevin responded before I finished writing my comment! I was going to propose a few alternatives, concretely illustrating what I meant by "don't keep creating new transports, just reuse the first one", but I think Kevin's approach is simpler. His way definitely aligns more closely with your existing mental model.

I will now just offer this: Taken exactly as written, Kevin's solution requires you to change all your current DatabaseTransport calls, of which there are many. Instead of

itransport = DatabaseTransport(self.conn)

you have to do

itransport = UnclosingDatabaseTransport(self.conn)

Not horrible by any means, but if you would like to disrupt your existing code as little as possible (creating the smallest diffs), you can make the following refinement:

from itoolkit.transport import DatabaseTransport as BaseDatabaseTransport

class DatabaseTransport(BaseDatabaseTransport):
    def _close(self):
        """Don't close connection, we'll manage it ourselves"""
        pass

Then you don't have to touch any of your other code.

[changlexc]

Great thanks for you help! I will try it on Ansible. One more question, I want to my code change both worked for itoolkit 1.6.x (without the close conn code)and itoolkit 1.7, any suggestions for my implementation?

[jkyeung]

Great thanks for you help! I will try it on Ansible. One more question, I want to my code change both worked for itoolkit 1.6.x (without the close conn code)and itoolkit 1.7, any suggestions for my implementation?

The approach suggested in this thread will work with 1.6.x and 1.7.x.

For your own understanding, I strongly encourage you to (a) try it out with itoolkit 1.6.x to see what happens, and (b) figure out for yourself why it does what it does (or doesn't do what it doesn't do). Hint: Instead of making the new _close method just pass, try putting in a print statement or raising an exception.

I think it's important to understand any code that you're going to commit into a project.

[changlexc]

Thanks for all your support, the solution is perfect. Both worked for itoolkit 1.6 and higher version.