Modify

Opened 15 years ago

Closed 8 years ago

#5552 closed defect (wontfix)

Reviewer unable to approve review: "ProgrammingError: Cannot operate on a closed cursor."

Reported by: thomas.peiselt@… Owned by: Ryan J Ollos
Priority: normal Component: PeerReviewPlugin
Severity: normal Keywords: closed, cursor, ProgrammingError
Cc: Steffen Hoffmann Trac Release: 0.11

Description

played around with different editions from subversion, but to no avail:

Sysinfo:

Trac:  	0.11.5
Python: 	2.4.4 (#71, Oct 18 2006, 08:34:43) [MSC v.1310 32 bit (Intel)]
setuptools: 	0.6c9
SQLite: 	3.6.11
pysqlite: 	2.5.5
Genshi: 	0.5.1
jQuery:	1.2.6

This is running on a Windows XP 64 SP2, but to me it seems like a database issue.

Traceback:

18:36:26 Trac[main] ERROR: Internal Server Error:
Traceback (most recent call last):
  File "C:\Program Files (x86)\Python2.4\Lib\site-packages\trac\web\main.py", l
ne 444, in _dispatch_request
    dispatcher.dispatch(req)
  File "C:\Program Files (x86)\Python2.4\Lib\site-packages\trac\web\main.py", l
ne 205, in dispatch
    resp = chosen_handler.process_request(req)
  File "build\bdist.win32\egg\codereview\peerReviewView.py", line 165, in proce
s_request
  File "build\bdist.win32\egg\codereview\peerReviewView.py", line 209, in vote
  File "build\bdist.win32\egg\codereview\ReviewerStruct.py", line 49, in save
  File "C:\Program Files (x86)\Python2.4\Lib\site-packages\trac\db\util.py", li
e 59, in execute
    return self.cursor.execute(sql_escape_percent(sql), args)
  File "C:\Program Files (x86)\Python2.4\Lib\site-packages\trac\db\sqlite_backe
d.py", line 58, in execute
    args or [])
  File "C:\Program Files (x86)\Python2.4\Lib\site-packages\trac\db\sqlite_backe
d.py", line 50, in _rollback_on_error
    return function(self, *args, **kwargs)
ProgrammingError: Cannot operate on a closed cursor.

Attachments (0)

Change History (13)

comment:1 Changed 15 years ago by isaac.to@…

I see the same problem. If I move the cursor = db.cursor(); line in codereview.ReviewerStruct.save() into both the try and except block, the problem goes away. Is that a right fix?

comment:2 Changed 13 years ago by quinn@…

I have the same issue with Trac 0.11.6 and the latest PeerReviewPlugin from http://trac-hacks.org/browser/peerreviewplugin/0.11.

Is development on the plugin dead? What is trac-hacks.org's process for submitting patches into release versions?

Quinn

comment:3 in reply to:  2 Changed 13 years ago by Ryan J Ollos

Replying to quinn@strangecode.com:

Is development on the plugin dead? What is trac-hacks.org's process for submitting patches into release versions?

You can request to adopt maintainership, see AdoptingHacks.

Alternatively, we can just apply some patches if no response from the author for 2 weeks. Ping me with a reminder if we don't hear anything and you'd like to proceed.

comment:4 Changed 13 years ago by Ryan J Ollos

#6184 closed as a duplicate.

comment:5 Changed 13 years ago by Ryan J Ollos

See also #7504.

comment:6 Changed 13 years ago by Ryan J Ollos

Owner: changed from Sebastian Marek to Ryan J Ollos
Status: newassigned

Checking in a proposed fix. Please reopen if you continue to have issues after installing from the trunk.

comment:7 Changed 13 years ago by Ryan J Ollos

Resolution: fixed
Status: assignedclosed

(In [9639]) Attempt to prevent cursor from being closed before it is used. Fixes #5552.

comment:8 Changed 12 years ago by Ryan J Ollos

Keywords: closed cursor added

comment:9 Changed 12 years ago by Ryan J Ollos

Resolution: fixed
Status: closedreopened

The 2.2-dev branch has been moved to the trunk, and the change in [9639] has not yet been applied there: peerreviewplugin/trunk/codereview/ReviewerStruct.py@3503.

comment:10 Changed 12 years ago by Ryan J Ollos

Cc: Steffen Hoffmann added; anonymous removed

Comments in comment:13:ticket:9521 lead me to believe that the fix in [9639] is not correct, because the cursor would be closed when the except block is executed. It looks like we must update the code to obtain a new cursor in each block:

    def save(self, db):
        try:
            #Add information to a new database entry
            cursor = db.cursor();
            cursor.execute("""
                INSERT INTO Reviewers (IDReview, Reviewer, Status, Vote)
                VALUES (%s, %s, %s, %s)""",
                (self.IDReview, self.Reviewer, self.Status, self.Vote))
            db.commit()
        except:
            #Update information in existing database entry
            cursor = db.cursor();
            cursor.execute("""
                UPDATE Reviewers SET Status=%s, Vote=%s
                WHERE IDReview=%s AND Reviewer=%s""",
                (self.Status, self.Vote, self.IDReview, self.Reviewer))
            db.commit()

However, I question whether we really want to be using a try/except here for handling an UPDATE vs INSERT. At a minimum, we should be handling a specific exception for this except case, and then add a generic exception handler for unexpected exceptions. I'll search around the Trac codebase a bit and see if some similar patterns exist.

comment:11 in reply to:  10 Changed 12 years ago by Steffen Hoffmann

Replying to rjollos:

Comments in comment:13:ticket:9521 lead me to believe that the fix in [9639] is not correct, because the cursor would be closed when the except block is executed.

...

However, I question whether we really want to be using a try/except here for handling an UPDATE vs INSERT. At a minimum, we should be handling a specific exception for this except case, and then add a generic exception handler for unexpected exceptions. I'll search around the Trac codebase a bit and see if some similar patterns exist.

Yes, indeed. The try-except shouldn't be needed here. I've taken advise from rblank to use a pattern of SQL statements, that he called UPSERT. It's an atomic transaction containing the following common structure:

  • unconditional UPDATE (will silently fail, if dataset isn't there
  • SELECT and check, if updated dataset exists
  • if no-match, the INSERT is safe

all done in one move - "atomic" transaction - and without exception handler. I've used that i.e. in AcctMgr, but it's done this way in many more plugins, and Trac core of course. It seems rather big at first sight, but it's performance is ok, and I've never had issues with code, that implements this strategy. Only the 'no-match' check is a bit trick to get, because different backends return different empty cursors. I prefer a two-step approach like (pre-0.12 db query style)

    db = _get_db(env, db)
    cursor = db.cursor()
    cursor.execute("""
        SELECT COUNT(*)
          FROM ...
        """)
    exists = cursor.fetchone()
    if not exists[0]:
        cursor.execute("""
            INSERT INTO ...
            """, (*args))

comment:12 Changed 10 years ago by Ryan J Ollos

Keywords: ProgrammingError added

comment:13 Changed 8 years ago by Cinc-th

Resolution: wontfix
Status: reopenedclosed

The plugin code doing all the database handling was rewritten by now and targets Trac version 0.12 and higher. The voting feature was entirely removed.

Thus this defect is obsolete.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.