Edgewall Software

Opened 11 years ago

Last modified 21 months ago

#11189 closed defect

ValueError: need more than 1 value to unpack — at Version 44

Reported by: anonymous Owned by: Ryan J Ollos
Priority: low Milestone: 1.0.2
Component: general Version: 0.12.5
Severity: normal Keywords: exception
Cc: Jun Omae Branch:
Release Notes:

Exceptions encountered when authenticating are trapped, logged and a warning is displayed. Previously, a traceback would be displayed in the browser. No additional authenticators are tried after an exception is encountered.

API Changes:

TimeoutError exception now inherits from TracError.

Internal Changes:

Description

How to Reproduce

While doing a GET operation on , Trac issued an internal error.

(please provide additional details here)

Request parameters:

{}

User agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:20.0) Gecko/20100101 Firefox/20.0

System Information

System information not available

Enabled Plugins

Plugin information not available

Python Traceback

Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/trac/web/main.py", line 522, in _dispatch_request
    dispatcher.dispatch(req)
  File "/usr/lib/python2.7/dist-packages/trac/web/main.py", line 208, in dispatch
    chosen_handler)
  File "/usr/lib/python2.7/dist-packages/trac/web/main.py", line 350, in _pre_process_request
    chosen_handler = filter_.pre_process_request(req, chosen_handler)
  File "/usr/lib/python2.7/dist-packages/trac/versioncontrol/api.py", line 330, in pre_process_request
    repo = self.get_repository(reponame)
  File "/usr/lib/python2.7/dist-packages/trac/versioncontrol/api.py", line 515, in get_repository
    repoinfo = self.get_all_repositories().get(reponame, {})
  File "/usr/lib/python2.7/dist-packages/trac/versioncontrol/api.py", line 583, in get_all_repositories
    for reponame, info in provider.get_repositories() or []:
  File "/usr/lib/python2.7/dist-packages/trac/versioncontrol/api.py", line 112, in get_repositories
    db = self.env.get_db_cnx()
  File "/usr/lib/python2.7/dist-packages/trac/env.py", line 329, in get_db_cnx
    return get_read_db(self)
  File "/usr/lib/python2.7/dist-packages/trac/db/api.py", line 91, in get_read_db
    or DatabaseManager(env).get_connection())
  File "/usr/lib/python2.7/dist-packages/trac/db/api.py", line 152, in get_connection
    connector, args = self.get_connector()
  File "/usr/lib/python2.7/dist-packages/trac/db/api.py", line 185, in get_connector
    scheme, args = _parse_db_str(self.connection_uri)
  File "/usr/lib/python2.7/dist-packages/trac/db/api.py", line 221, in _parse_db_str
    scheme, rest = db_str.split(':', 1)
ValueError: need more than 1 value to unpack

Change History (52)

comment:1 by anonymous, 11 years ago

miss: my OS is Ubuntu 13.04

comment:2 by Christian Boos, 11 years ago

Milestone: next-stable-1.0.x
Priority: normallow

Please double check the value you gave to the [trac] database setting.

This is an InstallationIssue, but we could show a nicer error message here.

comment:3 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Cc: ryan.j.ollos@… added

Patch proposed in t11189. The special case of an empty string is handled in c2bd917d to prepare a nicer error message, even though it would also be caught by the try/except/raise added in 581337ad.

comment:4 by Ryan J Ollos, 11 years ago

Milestone: next-stable-1.0.x1.0.2
Owner: set to Ryan J Ollos
Status: newassigned

by Ryan J Ollos, 11 years ago

Attachment: UnknownScheme.png added

by Ryan J Ollos, 11 years ago

Attachment: CantGetConnection.png added

by Ryan J Ollos, 11 years ago

Attachment: InvalidFormatNoRest.png added

by Ryan J Ollos, 11 years ago

Attachment: InvalidFormatNoColon.png added

by Ryan J Ollos, 11 years ago

Attachment: EmptyString.png added

comment:5 by Ryan J Ollos, 11 years ago

I've expanded the changeset to cover all the cases.

  1. Invalid format - no colon in connection string:

  1. Invalid format - string terminates at the colon:

  1. Empty connection string:

  1. Unknown scheme: (check already existed, but error message was enhanced)

  1. If the "non-scheme" part of the string is invalid, such as an invalid path to the SQLite database file, we still get an ugly traceback. However, raising a TracError for this issue probably needs to be handled outside of _parse_db_str and therefore may be outside the scope of this ticket.

comment:6 by Jun Omae, 11 years ago

Looks good! BTW, The similar errors occur by double port numbers, e.g. postgres://localhost:42:42 and malformed parameters, e.g. postgres://localhost/schema?name. Could you please try to fix these?

$ PYTHONPATH=. ~/venv/py25/bin/python
Python 2.5.6 (r256:88840, Feb 29 2012, 04:03:24)
[GCC 4.1.2 20080704 (Red Hat 4.1.2-51)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from trac.db.api import _parse_db_str
>>> _parse_db_str('postgres://localhost:5432:5432')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "trac/db/api.py", line 387, in _parse_db_str
    host, port = host.split(':')
ValueError: too many values to unpack
>>> _parse_db_str('postgres://localhost/schema?name')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "trac/db/api.py", line 404, in _parse_db_str
    name, value = param.split('=', 1)
ValueError: need more than 1 value to unpack

in reply to:  6 comment:7 by Ryan J Ollos, 11 years ago

Replying to jomae:

Looks good! BTW, The similar errors occur by double port numbers, e.g. postgres://localhost:42:42 and malformed parameters, e.g. postgres://localhost/schema?name. Could you please try to fix these?

Yeah, I'll cover those cases and post the changes for feedback. Thanks for reviewing!

comment:8 by Ryan J Ollos, 11 years ago

Btw, do you have any hints on the best way to push from Git to SVN? I've tried adding svn.edgewall.org/repos/trac as a remote to in my Git repository, but then it wants to pull down the entire history.

comment:9 by Jun Omae, 11 years ago

Try -r option for git svn fetch, e.g. git svn fetch -rXXXX:HEAD.

  1. Add the following to .git/config
    [svn-remote "svn"]
    	url = http://svn.edgewall.com/repos/trac
    	fetch = trunk:refs/remotes/svn/trunk
    	branches = branches/{0.12-stable,1.0-stable}:refs/remotes/svn/*
    
  2. Run git svn fetch -r11000:HEAD, it will create remotes/svn/{trunk,1.0-stable,0.12-stable} branches.

See http://stackoverflow.com/a/10173516/709074.

comment:10 by Christian Boos, 11 years ago

Well, for tracking the svn branches, you could directly fetch from the mirror (http://svn.edgewall.org/git/trac/mirror)

in reply to:  10 ; comment:11 by Ryan J Ollos, 11 years ago

Replying to cboos:

Well, for tracking the svn branches, you could directly fetch from the mirror (http://svn.edgewall.org/git/trac/mirror)

At the moment I'm primarily concerned with how to push from my Git branches to SVN. Do you and Jun both use git-svn?

in reply to:  11 ; comment:12 by Christian Boos, 11 years ago

Replying to rjollos:

Replying to cboos:

Well, for tracking the svn branches, you could directly fetch from the mirror (http://svn.edgewall.org/git/trac/mirror)

At the moment I'm primarily concerned with how to push from my Git branches to SVN. Do you and Jun both use git-svn?

I don't use it, when I "push to svn", I either commit what's on the branch wholesale or if I want to keep a svn changeset for each git commit, I do something like the following, assuming a tXYZ branch which has 4 commits on top of the last in svn (rebasing it first if needed):

$ git status -sb
## tXYZ
$ git checkout tXYZ~3
$ git log --format="%B" > commit.txt
$ svn ci -F commit.txt
$ git checkout tXYZ~2
...
$ git checkout tXYZ~1
...
$ git checkout tXYZ
...

(I use aliases to make the above more natural, st, co, msg).

I'm sure git svn could automate the above somehow, but I like to have this level of control, often tweaking commit.txt and running the test suite before committing to svn.

comment:13 by Ryan J Ollos, 11 years ago

I've prepared a series of changes that I think will address the issues Jun raised in comment:6, t11189.2. I'll wait a few days before committing in case that anyone has a chance to review.

I was thinking that I'd squash these changes to a single changeset before committing to SVN, but I'd be interested to hear opinions on that as well.

comment:14 by Peter Suter, 11 years ago

In #8401 the message Database Connection string must start with scheme:/ was replaced by Unknown scheme "%s"; database connection string must start with {scheme}:/'. (Although it seems like a known scheme with missing / would also trigger this message.)

Now you use the generic Invalid format "%(db_str)s" for database connection string. instead. Would it be better to again add additional hints about what exactly is invalid? E.g.:

  • It must start with {scheme}: but no : was found.
  • It must start with {scheme}:/ or sqlite: but no / was found and scheme was "%(scheme)s".
  • It contained host "%(host)s" but "%(port)s" is not a port number.
  • It contained parameters "%(qs)s" but no = was found in parameter "%(param)s".

About squashing related changes to a single changeset before committing to SVN: To me it seems like this has been a common practice in Trac. So it would not be surprising. But I'm not sure if it's really preferred / recommended and what the advantage is. Maybe it was just done to minimize the work with SVN before HG and Git made smaller changesets more practical?

To read everything at once one can still do things like log:trunk/trac/db@10406:10410 and diff:trunk/trac/db@10406:10410

in reply to:  14 comment:15 by Ryan J Ollos, 11 years ago

Replying to psuter:

[…] Now you use the generic Invalid format "%(db_str)s" for database connection string. instead. Would it be better to again add additional hints about what exactly is invalid? E.g.:

  • It must start with {scheme}: but no : was found.
  • It must start with {scheme}:/ or sqlite: but no / was found and scheme was "%(scheme)s".
  • It contained host "%(host)s" but "%(port)s" is not a port number.
  • It contained parameters "%(qs)s" but no = was found in parameter "%(param)s".

Thank you for the feedback. Overall, I felt that the method was overly complex and was looking for ways to simplify the code. I spent a bit of time trying to parse the string with a regular expression, but I couldn't make it work well.

When I removed the Invalid format error message and replaced it with a more generic one, I was mainly thinking that a great amount of detail shouldn't be needed in order for the user to figure out what is wrong. We're (hopefully) trapping the exception and presenting the user with a ConfigurationError for all of the cases in which the connection string is incorrect, and the user should be able to go to the documentation to figure out what is wrong.

It would certainly be more user-friendly to show the error messages you suggest, it just adds more code and messages to be translated. If we can help a few users though, it is probably worth adding the additional messages (and #8401 is certainly a good argument for that).

About squashing related changes to a single changeset before committing to SVN: To me it seems like this has been a common practice in Trac. So it would not be surprising. But I'm not sure if it's really preferred / recommended and what the advantage is. Maybe it was just done to minimize the work with SVN before HG and Git made smaller changesets more practical?

For the moment I just want to making sure that I'm falling in line with the way things are done in Trac since I think it is most important for consistent processes to be followed. If it was entirely up to me though, I can see the value of pushing the changes at the granularity of the Git development branch (8 changesets so far), but then merging them to trunk in a single changeset to keep the merge simple and the change history of the trunk less verbose.

comment:16 by Peter Suter, 11 years ago

I collected a few real world examples of invalid DB strings:

Error Report DB string
TracError: Database connection string must start with scheme:/ mysql:username:password@127.0.0.1:3306/trac
TracError: Database connection string must start with scheme:/ mysql:db/trac.db
TracError: Database Connection string must start with scheme:/ sqllite:db/trac.db
ValueError: invalid literal for int(): unix-socket= mysql://root:mercury@localhost:unix-socket=/home/mysql/mysql.sock/trac
ValueError: invalid literal for int() with base 10: '' mysql://login:ntrac123@localhost:/ntc_trac'
ValueError: invalid literal for int(): 1X2 the DB password starts with "1X2/" (Fixed in Trac: unquote password and username)
ValueError: too many values to unpack postgres://trac@tracdb@localhost:5432/trac?schema=local_projects
ValueError: too many values to unpack The '@' between the password and database address was missing
ValueError: too many values to unpack mysql://admin:secretpass:3306/trac
ValueError: need more than 1 value to unpack db/trac.db
ValueError: need more than 1 value to unpack Unknown
ValueError: need more than 1 value to unpack Unknown
ValueError: need more than 1 value to unpack Unknown
ValueError: need more than 1 value to unpack Unknown
AttributeError: 'NoneType' object has no attribute 'split' sqlite:db/trac.db(Different problem?)
AttributeError: 'NoneType' object has no attribute 'split' sqlite:db/trac.db(Different problem?)

I think you might be right. Including the db string will already help a lot. Any additional hints would probably confuse about as often as they would help.

Would it make sense to instead include a link to the documentation? (DatabaseBackend?)

in reply to:  16 comment:17 by Ryan J Ollos, 11 years ago

Replying to psuter:

Would it make sense to instead include a link to the documentation? (DatabaseBackend?)

I think so, and should the link specifically point to documentation on t.e.o since any local documentation will likely be unavailable?

Thanks for gathering up all of those examples. I'll take a closer look.

comment:18 by Christian Boos, 11 years ago

Well, the link could point to TracEnvironment#DatabaseConnectionStrings, and that section be expanded a bit if needed, notably to mention how an error would manifest itself.

in reply to:  12 comment:19 by Ryan J Ollos, 11 years ago

Replying to cboos:

I don't use it, when I "push to svn", I either commit what's on the branch wholesale or if I want to keep a svn changeset for each git commit, I do something like the following, assuming a tXYZ branch which has 4 commits on top of the last in svn (rebasing it first if needed):

Those steps worked well for me while committing to #11245. I found one additional step necessary, to copy the .svn directory from an SVN working copy of 1.0-stable into root of the Git repository. It's a nice way to apply patches from Git to SVN. Thanks!

comment:20 by Ryan J Ollos, 11 years ago

I'll be returning to this ticket shortly, and I'm thinking that I might rework the changes to utilize the generic ConfigurationError exception with its default message, as shown in comment:4:ticket:11272. This is to avoid exposing configuration data to the end user, such as Invalid format sqlite/db/trac.db.

On the one hand it doesn't seem very harmful to expose this information to users, and it may be more convenient to just present the information to the administrator since the situation is most likely to be encountered while setting up the Trac instance. On the other hand, presenting the ConfigurationError with the default message could be consistently used across Trac, and in addition to consistency we don't have to make a decision each time a similar situation arises and decide whether the configuration data should be exposed to all users.

A third option would be a future enhancement to only show a custom error message containing configuration data (i.e. the log message) through the web interface when the user has TRAC_ADMIN (discussion in bloodhound:#589 is related). I haven't explored whether this is practical for all situations - whether a permission check will be possible in all scenarios including the one presented in bloodhound:#589.

I appreciate any comment that will help me make the right decision on this small detail.

Last edited 11 years ago by Ryan J Ollos (previous) (diff)

in reply to:  20 ; comment:21 by Peter Suter, 11 years ago

Replying to rjollos:

only show a custom error message containing configuration data (i.e. the log message) through the web interface when the user has TRAC_ADMIN

That sounds interesting. A nice tradeoff. In situations where it's not easily possible to check permissions, we could default to not showing the details.

But maybe this complexity is not really needed. Encouraging more people to check the logs doesn't sound so bad either.

Do we have some TracDev page where the preferred approach to (not) showing configuration details in warning and error messages is (or could be) documented?

in reply to:  21 ; comment:22 by Ryan J Ollos, 11 years ago

Replying to psuter:

Replying to rjollos:

only show a custom error message containing configuration data (i.e. the log message) through the web interface when the user has TRAC_ADMIN

That sounds interesting. A nice tradeoff. In situations where it's not easily possible to check permissions, we could default to not showing the details.

But maybe this complexity is not really needed. Encouraging more people to check the logs doesn't sound so bad either.

I'm starting to think the complexity isn't needed for most cases. If there is demand for viewing the details through the web interface, maybe we could add a feature like th:LogViewerPlugin.

Do we have some TracDev page where the preferred approach to (not) showing configuration details in warning and error messages is (or could be) documented?

I haven't seen it documented anywhere, but I'd find documentation useful. Should we add a TracExceptions page, or is a broader category more appropriate? I'd find it useful to have the exception hierarchy displayed somewhere (PEP-0249 is a good example), and put use details in each Exception class. ConfigurationError could describe the proper type of information to expose in the exception message.

I was also planning to review the places where exceptions are raised and see if there are places where it makes sense to replace a generic TracError with a ConfigurationError.

in reply to:  22 comment:23 by Peter Suter, 11 years ago

I started TracDev/Exceptions. Feel free to improve on that.

Adding more details to the exception class docstrings sounds good.

comment:24 by Ryan J Ollos, 11 years ago

I've been using ConfigurationError fairly liberally for issues that are due to improper configuration of Trac. For example, in authz_policy.py a ConfigurationError is raised for:

  • 173: a missing [authz_policy] authz_file configuration option in trac.ini
  • 183: an authz file that can't be accessed (likely the file doesn't exist or it is a permissions issue)
  • 189: a missing package dependency (this will probably never be encountered after [10237] since the package is a setup.py requirement for the module, but I couldn't absolutely convince myself of that and it seemed harmless to leave it in).
  • 198: exceptions raised by ConfigObj while parsing the authz file
  • 201: an empty authz file

However, since ConfigurationError is defined in the config module, your narrower definition seems more valid: Exception raised when a value in the configuration file is not valid. Maybe I should be using a ParseError for 198 and 201? ParseError should probably be located in a different module such as trac.util in that case. Maybe we should have an InstallationError exception for issues such as 183 and 189?

Maybe it doesn't matter at all and I'm worrying about something irrelevant? At this point, the major differentiator I can see is having a more self-explanatory title when the exception is presented. As comment:4:ticket:11272 shows a Configuration Error, the InstallationError exception could display Installation Error.

Last edited 11 years ago by Ryan J Ollos (previous) (diff)

comment:25 by Peter Suter, 11 years ago

Note that I merely copied the existing docstring. If a broader definition would be more useful we could probably change it.

Reusing ParseError (Exception thrown for parse errors in authz files) for the 198 / 201 authz parsing issues seems fitting. But I don't see why it should be moved.

Restricting ConfigurationError to trac.config (i.e. TracIni) sounds logical. Not sure if it's a problem to reuse it for other kinds of configuration / installation issues. A new InstallationError sounds like a good alternative.

Maybe it's a bad idea to change the class hierarchy now, but would it make sense to insert it below the other two?

  • TracError
    • InstallationError: There is an issue with the Trac installation.
      • ConfigurationError: There is an issue in the TracIni configuration file.
      • ParseError: There is an issue preventing parsing of an authz file.

Not sure if the name is confusing though (suggesting an error occurred during installation.)

comment:26 by Peter Suter, 11 years ago

Keywords: exception added

by Ryan J Ollos, 10 years ago

by Ryan J Ollos, 10 years ago

by Ryan J Ollos, 10 years ago

comment:27 by Ryan J Ollos, 10 years ago

Here are the error messages that are presented for the 3 cases in which a ConfigurationError is raised:

  1. Empty database connection string:
  2. Database connection string that can't be parsed:
  3. Invalid path in SQLite database connection string:

Some comments:

  • I moved a string to module scope so that it could be reused. I'm not sure if this is a good idea: [cd81c49c/rjollos.git]
  • The tt tags will need to be changed to code tags when merging to the trunk (#11094).
  • There is some discussion in this ticket about not exposing configuration information to the user. Further steps could be taken to hide the information from end-users, but it doesn't seem all that bad to expose this bit of configuration information to users because the situation is likely to be encountered only during the initial installations or after a major configuration change, and since the problem is raised quite often in tickets and on the mailing list, my thinking is that it is pain point for inexperienced users and we should make the problem and fix as obvious as possible.
  • Another issue that I plan to address in a future ticket is stripping out the markup in the log messages to make them more readable. For example, maybe we can have a few rules such as replacing emphasis and tt/code tags with quotes, and putting hrefs after the text node:
    05:42:23 PM Trac[main] ERROR: can't retrieve session:
    ConfigurationError: Database connection string is empty.
    Set the <tt>database</tt> configuration option in the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracIni#trac-section" title="TracIni documentation">[trac]</a> section of trac.ini.
    Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracIni#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.
    
    05:42:23 PM Trac[main] ERROR: can't retrieve session:
    ConfigurationError: Database connection string is empty.
    Set the "database" configuration option in the [trac] (http://trac.edgewall.org/wiki/TracIni#trac-section) section of trac.ini.
    Please refer to the documentation (http://trac.edgewall.org/wiki/TracIni#DatabaseConnectionStrings) for help.
    
    This is also useful because the user is likely to encounter these exceptions while running trac-admin initenv:
    Initenv for '/home/user/Workspace/t11189/teo-rjollos.git/tracdev-pg' failed. 
    Failed to create environment.
    SQLite database not found at <tt>/home/user/Workspace/t11189/teo-rjollos.git/tracdev-pg/db/trac.db</tt>. Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracIni#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.
    Traceback (most recent call last):
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/admin/console.py", line 456, in do_initenv
        options=options)
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/core.py", line 124, in __call__
        self.__init__(*args, **kwargs)
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/env.py", line 280, in __init__
        self.create(options)
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/env.py", line 580, in create
        DatabaseManager(self).init_db()
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 249, in init_db
        connector, args = self.get_connector()
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 329, in get_connector
        path=tag.tt(args['path']), doc=_db_str_doc))
    ConfigurationError: SQLite database not found at <tt>/home/user/Workspace/t11189/teo-rjollos.git/tracdev-pg/db/trac.db</tt>. Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracIni#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.
    
    The last issue really makes me question whether it is worth having nice markup in the error message. If the error will usually be encountered during trac-admin initenv calls, then the markup will just make the message less readable.
  • One major issue remains unhandled. If the username/password/hostname fragment can't be correctly parsed, then an error is no longer raised, but we'll see a traceback when trying to open the db connection:
    >>> _parse_db_str('database = postgres://trac@trac@localhost:5432/trac')
    ('database = postgres', {'path': '/trac', 'host': 'trac@localhost', 'user': 'trac', 'port': 5432})
    
    Traceback (most recent call last):
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/api.py", line 524, in send_error
        data, 'text/html')
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/chrome.py", line 983, in render_template
        message = Markup(req.session.pop('chrome.%s.%d'
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/api.py", line 321, in __getattr__
        value = self.callbacks[name](self)
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/main.py", line 268, in _get_session
        return Session(self.env, req)
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/session.py", line 207, in __init__
        self.get_session(sid)
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/session.py", line 230, in get_session
        super(Session, self).get_session(sid, authenticated)
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/session.py", line 77, in get_session
        with self.env.db_query as db:
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 175, in __enter__
        db = self.dbmgr.get_connection(readonly=True)
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 263, in get_connection
        db = self._cnx_pool.get_cnx(self.timeout or None)
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/pool.py", line 213, in get_cnx
        return _backend.get_cnx(self._connector, self._kwargs, timeout)
      File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/pool.py", line 134, in get_cnx
        raise TimeoutError(errmsg)
    TimeoutError: Unable to get database connection within 20 seconds. (OperationalError: could not translate host name "trac@localhost" to address: System error
    )
    
    Should we try to trap the TimeoutErrors?:
    • trac/db/api.py

      diff --git a/trac/db/api.py b/trac/db/api.py
      index 8b59d24..4cc43da 100644
      a b from trac.util.concurrency import ThreadLocal  
      2727from trac.util.text import unicode_passwd
      2828from trac.util.translation import _, tag_
      2929
      30 from .pool import ConnectionPool
       30from .pool import ConnectionPool, TimeoutError
      3131from .util import ConnectionWrapper
      3232
      3333
      class DatabaseManager(Component):  
      260260        if not self._cnx_pool:
      261261            connector, args = self.get_connector()
      262262            self._cnx_pool = ConnectionPool(5, connector, **args)
      263         db = self._cnx_pool.get_cnx(self.timeout or None)
       263        try:
       264            db = self._cnx_pool.get_cnx(self.timeout or None)
       265        except TimeoutError, e:
       266            raise TracError(e)
      264267        if readonly:
      265268            db = ConnectionWrapper(db, readonly=True)
      266269        return db
    I couldn't find any performance difference after the try/except was added when measuring calls to env.get_db_cnx with the timeit module.

An even simpler fix could be:

  • trac/db/pool.py

    diff --git a/trac/db/pool.py b/trac/db/pool.py
    index 2534067..ca84b15 100644
    a b from trac.util.text import exception_to_unicode  
    2626from trac.util.translation import _
    2727
    2828
    29 class TimeoutError(Exception):
     29class TimeoutError(TracError):
    3030    """Exception raised by the connection pool when no connection has become
    3131    available after a given timeout."""

However, it makes me wonder if it was intentional that TimeoutError not inherit from TracError. TracError existed when TimeoutError was added in [2503].

The latest proposed changes can be found in rjollos.git:t11189.3.

comment:28 by Ryan J Ollos, 10 years ago

Cc: ryan.j.ollos@… removed

in reply to:  27 comment:29 by Ryan J Ollos, 10 years ago

Replying to rjollos:

This is also useful because the user is likely to encounter these exceptions while running trac-admin initenv:

Initenv for '/home/user/Workspace/t11189/teo-rjollos.git/tracdev-pg' failed. 
Failed to create environment.
SQLite database not found at <tt>/home/user/Workspace/t11189/teo-rjollos.git/tracdev-pg/db/trac.db</tt>. Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracIni#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.
Traceback (most recent call last):
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/admin/console.py", line 456, in do_initenv
    options=options)
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/core.py", line 124, in __call__
    self.__init__(*args, **kwargs)
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/env.py", line 280, in __init__
    self.create(options)
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/env.py", line 580, in create
    DatabaseManager(self).init_db()
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 249, in init_db
    connector, args = self.get_connector()
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 329, in get_connector
    path=tag.tt(args['path']), doc=_db_str_doc))
ConfigurationError: SQLite database not found at <tt>/home/user/Workspace/t11189/teo-rjollos.git/tracdev-pg/db/trac.db</tt>. Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracIni#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.

The last issue really makes me question whether it is worth having nice markup in the error message. If the error will usually be encountered during trac-admin initenv calls, then the markup will just make the message less readable.

We could probably do a bit better overall in presentation of these excepted exception - catch the exception, print the exception error message (possibly after stripping out the markup) and avoid displaying the traceback in the console.

in reply to:  27 comment:30 by Jun Omae, 10 years ago

Replying to rjollos:

  • I moved a string to module scope so that it could be reused. I'm not sure if this is a good idea: [cd81c49c/rjollos.git]

That's not good idea. I confirmed that _("documentation") in the changes never be translated. Also, if i18n methods return Babel's LazyProxy object, the object will cache translated message. Then, we should create the string every time.

comment:31 by Ryan J Ollos, 10 years ago

The latest proposed changes can be found in log:rjollos.git:t11189.4:

  • Squashed previous set of commits and [cd81c49c/rjollos.git] was removed as suggested in comment:30.
  • TimeoutError inherits from TracError so that exceptions will be trapped.
  • If database connection can't be obtained and user has an authenticated session, a traceback still results. This can occurs, at least for LoginModule, while trying to retrieve the cookie from the database: trac.web.main:authenticatetrac.web.auth:authenticatetrac.web.auth:_get_name_for_cookietrac.web.auth:_cookie_to_name. I've proposed to fix this in [2698e6dd/rjollos.git], but I'm not sure it is the best way.

in reply to:  31 ; comment:32 by Peter Suter, 10 years ago

Replying to rjollos:

  • TimeoutError inherits from TracError so that exceptions will be trapped.

There are some other exception classes that don't inherit from TracError. Should (some of) these be changed as well?

in reply to:  32 comment:33 by Ryan J Ollos, 10 years ago

Replying to psuter:

There are some other exception classes that don't inherit from TracError. Should (some of) these be changed as well?

I'm not sure, but I after I saw your comment on the wiki I added to my TODO list as an issue to investigate. I thought I'd look at the cases where they are used and make sure to test the behavior when exceptions are raised. Based on the code I've worked with, ParseError seems like an obvious candidate to inherit from TracError. Probably some or all of the others as well.

I'll make sure to update the exception hierarchy at TracDev/Exceptions if the change in [11d384c7/rjollos.git] gets pushed.

comment:34 by Ryan J Ollos, 10 years ago

Checking for the existence of the SQLite database file in [f95af781/rjollos.git#file0] presents a problem because the connector is retrieved before the database is initialized: branches/1.0-stable/trac/db/api.py@12111:248,251#L222. The following traceback results:

$ trac-admin new initenv
Creating a new Trac environment at /home/user/Workspace/t11189/teo-rjollos.git/new

Trac will first ask a few questions about your environment
in order to initialize and prepare the project database.

 Please enter the name of your project.
 This name will be used in page titles and descriptions.

Project Name [My Project]> new

 Please specify the connection string for the database to use.
 By default, a local SQLite database is created in the environment
 directory. It is also possible to use an already existing
 PostgreSQL database (check the Trac documentation for the exact
 connection string syntax).

Database connection string [sqlite:db/trac.db]> 

Creating and Initializing Project
Initenv for '/home/user/Workspace/t11189/teo-rjollos.git/new' failed. 
Failed to create environment.
SQLite database not found at <tt>/home/user/Workspace/t11189/teo-rjollos.git/new/db/trac.db</tt>. Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracEnvironment#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.
Traceback (most recent call last):
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/admin/console.py", line 456, in do_initenv
    options=options)
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/core.py", line 124, in __call__
    self.__init__(*args, **kwargs)
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/env.py", line 280, in __init__
    self.create(options)
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/env.py", line 580, in create
    DatabaseManager(self).init_db()
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 249, in init_db
    connector, args = self.get_connector()
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 334, in get_connector
    path=tag.tt(args['path']), doc=doc))
ConfigurationError: SQLite database not found at <tt>/home/user/Workspace/t11189/teo-rjollos.git/new/db/trac.db</tt>. Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracEnvironment#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.

So far, the only ways I can think to resolve this are:

  • Remove the test for the path existence.
  • Add a preinit parameter to get_connector that determines whether the exception is raised.
  • Move the body of get_connector to a private method, but keep the path existence test in get_connector, allowing us to call the private method from init_db. Unfortunately _get_connector is already used for backward compatibility: branches/1.0-stable/trac/db/api.py?rev=12111&marks=329#L299
Last edited 10 years ago by Ryan J Ollos (previous) (diff)

in reply to:  31 ; comment:35 by Jun Omae, 10 years ago

Sorry for my late response.

  • TimeoutError inherits from TracError so that exceptions will be trapped.
  • If database connection can't be obtained and user has an authenticated session, a traceback still results. This can occurs, at least for LoginModule, while trying to retrieve the cookie from the database: trac.web.main:authenticatetrac.web.auth:authenticatetrac.web.auth:_get_name_for_cookietrac.web.auth:_cookie_to_name. I've proposed to fix this in [2698e6dd/rjollos.git], but I'm not sure it is the best way.

In [12306], if IAuthenticator component raises a TracError, RequestDispatcher.authenticate method would skip the component and try next. I think that's not good because it unexpectedly might use other authenticators.

  • trac/web/main.py

    diff --git a/trac/web/main.py b/trac/web/main.py
    index d668736..604f203 100644
    a b class RequestDispatcher(Component):  
    138138                self.log.error("Can't authenticate using %s: %s",
    139139                               authenticator.__class__.__name__,
    140140                               exception_to_unicode(e))
     141                raise
    141142            else:
    142143                if authname:
    143144                    return authname

comment:36 by Jun Omae, 10 years ago

Cc: Jun Omae added

in reply to:  35 ; comment:37 by Ryan J Ollos, 10 years ago

Replying to jomae:

In [12306], if IAuthenticator component raises a TracError, RequestDispatcher.authenticate method would skip the component and try next. I think that's not good because it unexpectedly might use other authenticators.

Re-raising the exception there results in the traceback for the case mentioned in comment:31. How about something like this?

  • trac/web/main.py

    diff --git a/trac/web/main.py b/trac/web/main.py
    index d668736..d16a5b4 100644
    a b from trac.util.text import exception_to_unicode, shorten_line,  
    4848from trac.util.translation import _, get_negotiated_locale, has_babel, \
    4949                                  safefmt, tag_
    5050from trac.web.api import *
    51 from trac.web.chrome import Chrome
     51from trac.web.chrome import Chrome, add_warning
    5252from trac.web.href import Href
    5353from trac.web.session import Session
    5454
    class RequestDispatcher(Component):  
    138138                self.log.error("Can't authenticate using %s: %s",
    139139                               authenticator.__class__.__name__,
    140140                               exception_to_unicode(e))
     141                add_warning(req, _("Authentication error. "
     142                                   "Please contact your administrator."))
    141143            else:
    142144                if authname:
    143145                    return authname
    144         else:
    145             return 'anonymous'
     146        return 'anonymous'
    146147
    147148    def dispatch(self, req):
    148149        """Find a registered handler that matches the request and let
Last edited 10 years ago by Ryan J Ollos (previous) (diff)

in reply to:  37 comment:38 by Jun Omae, 10 years ago

Replying to rjollos:

Re-raising the exception there results in the traceback for the case mentioned in comment:31. How about something like this?

Okay, two things.

  • We should log with traceback to solve it by administrator
  • Break the loop to do not fallback authenticator
  • trac/web/main.py

    diff --git a/trac/web/main.py b/trac/web/main.py
    index d668736..c8d92f2 100644
    a b from trac.util.text import exception_to_unicode, shorten_line, to_unicode  
    4848from trac.util.translation import _, get_negotiated_locale, has_babel, \
    4949                                  safefmt, tag_
    5050from trac.web.api import *
    51 from trac.web.chrome import Chrome
     51from trac.web.chrome import Chrome, add_warning
    5252from trac.web.href import Href
    5353from trac.web.session import Session
    5454
    class RequestDispatcher(Component):  
    137137            except TracError, e:
    138138                self.log.error("Can't authenticate using %s: %s",
    139139                               authenticator.__class__.__name__,
    140                                exception_to_unicode(e))
    141             else:
    142                 if authname:
    143                     return authname
    144         else:
    145             return 'anonymous'
     140                               exception_to_unicode(e, traceback=True))
     141                add_warning(req, _("Authentication error. "
     142                                   "Please contact your administrator."))
     143                break  # don't fallback authenticator
     144            if authname:
     145                return authname
     146        return 'anonymous'
    146147
    147148    def dispatch(self, req):
    148149        """Find a registered handler that matches the request and let

comment:39 by Ryan J Ollos, 10 years ago

Okay, good. That is pretty much the same as the patch I was testing locally. Somehow though I posted the wrong patch in comment:37, which didn't have the break.

comment:40 by Ryan J Ollos, 10 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

Changes from comment:37 committed to 1.0-stable in [12315] and merged to trunk in [12316].

in reply to:  34 ; comment:41 by Ryan J Ollos, 10 years ago

Replying to rjollos:

Checking for the existence of the SQLite database file in [f95af781/rjollos.git#file0] presents a problem because the connector is retrieved before the database is initialized: branches/1.0-stable/trac/db/api.py@12111:248,251#L222. The following traceback results: […] So far, the only ways I can think to resolve this are:

  • Remove the test for the path existence.
  • Add a preinit parameter to get_connector that determines whether the exception is raised.
  • Move the body of get_connector to a private method, but keep the path existence test in get_connector, allowing us to call the private method from init_db. Unfortunately _get_connector is already used for backward compatibility: branches/1.0-stable/trac/db/api.py?rev=12111&marks=329#L299

The latest proposed changes can be found in log:rjollos.git:t11189.7:

  • Added unit tests for RequestDispatcher.authenticate
  • Added a private _get_connector method, and the public get_connector method checks for path existence. The private _get_connector is called in init_db, which avoids the exception due to the check for existence of the SQLite database.

I am not entirely sure that the latter change is the best way to go, so I'll wait for feedback before committing.

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

in reply to:  41 comment:42 by Jun Omae, 10 years ago

  • Added a private _get_connector method, and the public get_connector method checks for path existence. The private _get_connector is called in init_db, which avoids the exception due to the check for existence of the SQLite database.

The changes break unit tests on PostgreSQL and MySQL. I think that might lead something wrong because DatabaseManager.get_connector() method is used from Trac plugins to generate DDL statements via to_sql(). Also, that is ineffective without check of writable permission for SQLite database. The issue often occurs if other than tracd web server.

I think it is difficult to validate perfectly database string without actual connecting database.

$ make python=25 db=postgres clean unit-test
...
TRAC_TEST_DB_URI=postgres://tracuser:password@localhost:5432/tractest?schema=tractest
...
psycopg2.ProgrammingError: schema "tractest" already exists
$ make python=25 db=mysql clean unit-test
...
TRAC_TEST_DB_URI=mysql://tracuser:password@localhost/tractest
...
_mysql_exceptions.OperationalError: (1050, "Table 'system' already exists")

comment:43 by anonymous, 10 years ago

offtopic: milestone is moving target. Do you think you could fix remaining 6 tickets and release?

comment:44 by Ryan J Ollos, 10 years ago

Release Notes: modified (diff)
Note: See TracTickets for help on using tickets.