New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected behaviour of __exit__ method #113

Closed
aalloul opened this Issue Nov 17, 2017 · 15 comments

Comments

Projects
None yet
5 participants
@aalloul
Copy link

aalloul commented Nov 17, 2017

Description:

I believe the __exit__() method does not behave as expected. PEP-343 states

Many context managers (such as files and generator-based contexts) will be single-use objects. Once the __exit__() method has been called, the context manager will no longer be in a usable state (e.g. the file has been closed, or the underlying generator has finished execution).

The current implementation commits or rolls-back any transaction but leaves the connection open.

More information

  1. What is your version of Python? Is it 32-bit or 64-bit?
    Python 3.6, 64 bits

  2. What is your version of cx_Oracle?
    '6.0b2'

  3. What is your OS and version?
    Mac OSX 10.12.6

  4. What compiler version did you use? For example, with GCC, run
    gcc --version.
    Apple LLVM version 9.0.0 (clang-900.0.38)

  5. What exact command caused the problem (e.g. what command did you try to
    install with)? Who were you logged in as?
    conn= cx_Oracle.connect(user=username,password=password,dsn=dsn) conn.__exit__()

  6. What error(s) you are seeing?
    The __exit__() method returns False without closing the connection. This is unexpected behaviour as users usually expect a with-block to always close the open connection. As an example,

f = open(filename, "r")
f.__exit__()
f.closed

returns True

@anthony-tuininga

This comment has been minimized.

Copy link
Member

anthony-tuininga commented Nov 17, 2017

This feature was added in version 4.3.3 of cx_Oracle, back in October 2007. I'm not sure how widely used it is, of course, but changing it now would be disruptive, I suspect. Unless I see a lot of +1's from people on this issue, I don't think it will get changed. If I was considering this feature now I'd likely go with your suggestion, but I think that ship has sailed. We'll see if others chime in. :-)

@aalloul

This comment has been minimized.

Copy link
Author

aalloul commented Nov 20, 2017

Hi Anthony,

thanks for your quick answer. I'm on the side of thinking that Python programmers expect the connection to be closed when exiting the context manager. That's the whole point of having it (i.e. avoiding the try-except-finally construct).

It might very well be that users are using the with statement without checking whether the connection is closed after exiting.

@anthony-tuininga

This comment has been minimized.

Copy link
Member

anthony-tuininga commented Nov 20, 2017

I understand. Hopefully others chime in here. I'll leave this issue open for a while to give others a chance to comment.

@marksteward

This comment has been minimized.

Copy link

marksteward commented Nov 20, 2017

FWIW, I don't use it because of this.

@anthony-tuininga

This comment has been minimized.

Copy link
Member

anthony-tuininga commented Nov 20, 2017

Thanks for letting us know.

@markfinn

This comment has been minimized.

Copy link

markfinn commented Dec 6, 2017

Since you're requesting opinions, I would say it needs to follow the PEP and definition of the with keyword. It needs to close the connection as would be expected by a user.

Depreciating a feature that can be worked around by rolling-your-own with try/finally is not a big deal compared to making a core language feature act as expected.

@anthony-tuininga

This comment has been minimized.

Copy link
Member

anthony-tuininga commented Dec 6, 2017

I am indeed requesting opinions. :-) Which PEP are you referring to? Does it explicitly mention database connections? If so, can you provide a link?

@markfinn

This comment has been minimized.

Copy link

markfinn commented Dec 7, 2017

I was referring to the PEP-343 quote in comment 1

@anthony-tuininga

This comment has been minimized.

Copy link
Member

anthony-tuininga commented Dec 7, 2017

Ah, ok. Thanks for clarifying.

@lcary

This comment has been minimized.

Copy link

lcary commented Feb 15, 2018

Since you're requesting opinions, I think it's very confusing that the with-statement does not close the connection.

Additionally confusing: I noticed that the docs imply that __del__ closes the connection:

Close the connection now, rather than whenever __del__ is called. 

However, if this was true, then I would expect the exiting of the with-statement to close the database connection, since exiting a with-statement will call __del__ on an object (demo).

My team is using this library for tools that thousands of users at Columbia University interact with daily. I think the above comment by @markfinn makes a lot of sense.

@anthony-tuininga

This comment has been minimized.

Copy link
Member

anthony-tuininga commented Feb 15, 2018

However, if this was true, then I would expect the exiting of the with-statement to close the database connection, since exiting a with-statement will call del on an object (demo).

Your demo does so only because you are creating the object in the with clause itself. If you create the object earlier like this, it behaves differently:

conn = Connection()
with conn:
    print("testing Connection...")
print("All done")

Then you will see that the "All done" gets called before the __del__ gets called.

All of that said, I can see that the general opinion is that it should close the connection, so I will look into what I can do in order to implement the new behaviour while retaining the old behaviour for those few that may have used it. I will also deprecate the old behaviour and make the new behaviour the default (and perhaps only) behaviour in cx_Oracle 7. I appreciate your comments.

@anthony-tuininga

This comment has been minimized.

Copy link
Member

anthony-tuininga commented Feb 16, 2018

I've given this some thought. I see a few options. Let me know your thoughts on them! Thanks.

  1. Create a special object called cx_Oracle.__future__ which has special attribute processing. The only attribute it would (currently) understand would be context_mgr_close (or some other more suitable name). Setting that value to True would make the transaction manager on the connection close the connection instead of commit/rollback the transaction as it currently does. Then in cx_Oracle 7 the behaviour would change to always do this. Setting other attributes in this special object would do nothing. This assumes that the other behaviour (commit/rollback a transaction) is not useful. This option assumes that a transaction context manager is not useful and should be removed.

  2. Create new methods pool.contextmgr() and connection.contextmgr() that behave in the desired way (close the connection). The pool method would acquire a connection from the pool and then return it back to the pool when the transaction manager is finished. The connection method would simply close itself. For connections acquired from the pool this would be the same as pool.contextmgr() in essence. Another method connection.transactioncontextmgr() would also be created to take the place of the old context manager functionality which would be deprecated and removed in cx_Oracle 7.

  3. Combine options 1 and 2 and create the special cx_Oracle.__future__ object but also create connection.transactioncontextmgr() for those who desire the old functionality. This would become the only method available to use the old functionality in cx_Oracle 7.

anthony-tuininga added a commit that referenced this issue Feb 27, 2018

Added support for closing the connection when reaching the end of a c…
…ode block

controlled by the connection as a context manager, but in a backwards
compatible way (#113).
@anthony-tuininga

This comment has been minimized.

Copy link
Member

anthony-tuininga commented Feb 28, 2018

I have just added a commit that causes the connection context manager to close the connection. To take advantage of this now (rather than in cx_Oracle 7), put the following somewhere in your code before you use the with clause:

cx_Oracle.__future__.ctx_mgr_close = True

In cx_Oracle 7 the old behaviour will simply be dropped unless someone specifically requests the old behaviour to be retained. Comments welcome!

@aalloul

This comment has been minimized.

Copy link
Author

aalloul commented Feb 28, 2018

Hi Anthony,

that's awesome news, thanks a lot!

I meant to reply to your previous comment but got really busy at work... Anyways, it would have been a comment to state that I'm not good at programming in C at all and I wouldn't know what the right implementation should be.

I'd much rather let someone else provide feedback!

Thanks again

@anthony-tuininga

This comment has been minimized.

Copy link
Member

anthony-tuininga commented Mar 5, 2018

This has been released as cx_Oracle 6.2 just now. Please open a new issue if you detect any issues with this functionality! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment