Fix possible bugs when setting sqlite3.Connection.isolation_level

[zhangyangyu]

BPO	27881
Nosy	@berkerpeksag, @serhiy-storchaka, @zhangyangyu, @palaviv
Files	issue27881.patch issue27881_v2.patch issue27881_v3.patch issue27881_v4.patch issue27881_v5.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-09-08.13:24:25.345>
created_at = <Date 2016-08-27.19:21:29.390>
labels = ['type-bug', 'library']
title = 'Fix possible bugs when setting sqlite3.Connection.isolation_level'
updated_at = <Date 2016-09-08.13:24:25.344>
user = 'https://github.com/zhangyangyu'

bugs.python.org fields:

activity = <Date 2016-09-08.13:24:25.344>
actor = 'berker.peksag'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-09-08.13:24:25.345>
closer = 'berker.peksag'
components = ['Library (Lib)']
creation = <Date 2016-08-27.19:21:29.390>
creator = 'xiang.zhang'
dependencies = []
files = ['44247', '44265', '44266', '44318', '44328']
hgrepos = []
issue_num = 27881
keywords = ['patch']
message_count = 14.0
messages = ['273796', '273820', '273909', '273910', '273914', '274068', '274088', '274100', '274101', '274158', '274163', '274168', '274169', '275013']
nosy_count = 6.0
nosy_names = ['python-dev', 'berker.peksag', 'serhiy.storchaka', 'xiang.zhang', 'palaviv', 'tehybel']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue27881'
versions = ['Python 3.5', 'Python 3.6']

[zhangyangyu]

This is a follow up of bpo-27861 and means to fix the second issue mentioned in it.

pysqlite_connection_set_isolation_level now does not check allowed values and when any part fails, it leaves the Connection object in an inconsistent state.

I'll write a patch trying to fix this tomorrow.

[zhangyangyu]

bpo-27881.patch tires to solve this. Hope to get feedback.

[zhangyangyu]

Serhiy, I've updated the patch. Looking forward to your feedback.

[serhiy-storchaka]

In general LGTM. But added several style comments.

[zhangyangyu]

Leave replies and make a trival change in v3. I don't change the other two just as I state in the replies. But please do what you like.

[palaviv]

Xiang what do you think about changing the isolation_levels list to begin_statements list:
static const char * const begin_statements[] = {"BEGIN", "BEGIN DEFERRED", "BEGIN IMMEDIATE", "BEGIN EXCLUSIVE", NULL};

This change will allow you to remove all the strings concatenations and the malloc/free.

[zhangyangyu]

I thought about this method but didn't find it's simpler.

You cannot avoid malloc/free since the isolation level has to be on heap. It's going to be freed in the dealloc method unless your alter that part too. Then it's about one memcpy or two, which doesn't make much difference. And in this method you have to do more in argument checking. A simple strstr doesn't solve this problem.

[serhiy-storchaka]

Xiang what do you think about changing the isolation_levels list to begin_statements list

This is interesting suggestion. Indeed, this simplifies the code. Thanks Aviv.

[zhangyangyu]

Hmm, you do this "It's going to be freed in the dealloc method unless your alter that part too". If this is done I admit this is more clean. Patch LGTM.

[palaviv]

What do you think about removing the isolation_level member from the pysqlite_Connection. It is only there to be for the pysqlite_connection_get_isolation_level and that could be easily calculated from the begin_statement.

[serhiy-storchaka]

Yes, I thought about this. This changes the behavior (for now isolation_level returns exactly the same object that was assigned to it) and slows down the getter. This can be added only on the default branch.

[palaviv]

The only change I see that can happen is that we return upper case isolation level when the user used a lower case. I think that it is worth to slow down the getter for the code simplification.