Edgewall Software
Modify

Opened 4 years ago

Last modified 4 years ago

#12004 new enhancement

Replace uses of assert with exception-raising statements

Reported by: Ryan J Ollos Owned by:
Priority: normal Milestone: next-major-releases
Component: general Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:

Description

It was noted in comment:7:ticket:11295 that we should consider replacing uses of assert, which will be removed when running the Python interpreter in optimized (-O) mode, with raises. Here is an example: tags/trac-1.0.5/trac/web/session.py@:242-244#L241.

There are a few possible paths we could pursue:

  1. Replace all uses of assert.
  2. Use assert to protect against coding errors, raise exceptions for errors that can be triggered by a user in the scenario of proper-functioning code.

Whichever approach we take, it should be captured in TracDev/CodingStyle.

Attachments (0)

Change History (6)

comment:1 by Ryan J Ollos, 4 years ago

Milestone: next-dev-1.1.xnext-major-releases
Type: defectenhancement

comment:2 by Ryan J Ollos, 4 years ago

th:comment:5:ticket:12292 suggests replacing the following assert with raising a TracError: ​browser:/tags/trac-1.0.6/trac/util/presentation.py@:176#L167 .

comment:3 by Ryan J Ollos, 4 years ago

Found this error in the logs.

2015-07-21 20:42:28,547 Trac[main] ERROR: Internal Server Error:
Traceback (most recent call last):
  File "build/bdist.linux-i686/egg/trac/web/main.py", line 550, in _dispatch_request
    dispatcher.dispatch(req)
  File "build/bdist.linux-i686/egg/trac/web/main.py", line 253, in dispatch
    self._post_process_request(req, *resp)
  File "build/bdist.linux-i686/egg/trac/web/main.py", line 384, in _post_process_request
    resp = f.post_process_request(req, *resp)
  File "/srv/trac-hacks.org/pve/lib/python2.6/site-packages/TracVote-0.3.0-py2.6.egg/tracvote/__init__.py", line 391, in post_process_request
    self.render_voter(req)
  File "/srv/trac-hacks.org/pve/lib/python2.6/site-packages/TracVote-0.3.0-py2.6.egg/tracvote/__init__.py", line 598, in render_voter
    resource = resource_from_path(self.env, path)
  File "/srv/trac-hacks.org/pve/lib/python2.6/site-packages/TracVote-0.3.0-py2.6.egg/tracvote/__init__.py", line 93, in resource_from_path
    if _resource_exists(env, resource) in (None, True):
  File "/srv/trac-hacks.org/pve/lib/python2.6/site-packages/TracVote-0.3.0-py2.6.egg/tracvote/__init__.py", line 74, in _resource_exists
    return resource_exists(env, resource)
  File "build/bdist.linux-i686/egg/trac/resource.py", line 454, in resource_exists
    return manager.resource_exists(resource)
  File "build/bdist.linux-i686/egg/trac/ticket/api.py", line 604, in resource_exists
    if self.env.db_query("SELECT id FROM ticket WHERE id=%s", (id_,)):
  File "build/bdist.linux-i686/egg/trac/db/api.py", line 124, in execute
    return db.execute(query, params)
  File "build/bdist.linux-i686/egg/trac/db/api.py", line 184, in __exit__
    self.db.close()
  File "build/bdist.linux-i686/egg/trac/db/pool.py", line 51, in close
    self._pool._return_cnx(cnx, self._key, self._tid)
  File "build/bdist.linux-i686/egg/trac/db/pool.py", line 166, in _return_cnx
    assert (tid, key) in self._active
AssertionError:

I'm unsure of the exact conditions leading to the error, but since this can occur in production it would at least be more clear if we raised a TracError with a message: tags/trac-1.0.7/trac/db/pool.py@:166#L163

in reply to:  2 comment:4 by Ryan J Ollos, 4 years ago

Replying to Ryan J Ollos:

th:comment:5:ticket:12292 suggests replacing the following assert with raising a TracError: ​browser:/tags/trac-1.0.6/trac/util/presentation.py@:176#L167 .

Found in the logs today:

2015-11-08 16:26:46,105 Trac[main] ERROR: Internal Server Error: <RequestWithSession "GET '/search?wiki=on&q=%5Bdownload%3A&page=91&noquickjump=1'">, referrer None
Traceback (most recent call last):
  File "build/bdist.linux-i686/egg/trac/web/main.py", line 554, in _dispatch_request
    dispatcher.dispatch(req)
  File "build/bdist.linux-i686/egg/trac/web/main.py", line 247, in dispatch
    resp = chosen_handler.process_request(req)
  File "build/bdist.linux-i686/egg/trac/search/web_ui.py", line 107, in process_request
    data.update(self._prepare_results(req, filters, results))
  File "build/bdist.linux-i686/egg/trac/search/web_ui.py", line 218, in _prepare_results
    results = Paginator(results, page - 1, self.RESULTS_PER_PAGE)
  File "build/bdist.linux-i686/egg/trac/util/presentation.py", line 205, in __init__
    items, num_items, num_pages = paginate(items, page, max_per_page)
  File "build/bdist.linux-i686/egg/trac/util/presentation.py", line 176, in paginate
    assert start < count, 'Page %d out of range' % page
AssertionError: Page 90 out of range

The proposed fix may be similar to [th 14785].

comment:5 by Ryan J Ollos, 4 years ago

Proposed changes to address the issue in comment:4 in log:rjollos.git:t12004_paginate_exception.

comment:6 by Ryan J Ollos, 4 years ago

comment:5 changes committed to 1.0-stable in [14364:14365], merged to trunk in [14366].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned. Next status will be 'new'.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.