errors when importing WorkingTree on a thread due to signals

Bug #521989 reported by Elliot Murphy on 2010-02-15
34
This bug affects 5 people
Affects Status Importance Assigned to Milestone
Bazaar
High
Martin Pool
2.1
High
Martin Pool
Ubuntu One Servers
Low
Elliot Murphy
bzr (Ubuntu)
Undecided
Unassigned
Nominated for Lucid by Martin Pool

Bug Description

I have some python code that runs inside the paste threaded wsgi server. It imports WorkingTree just to figure out some simple things like the root of the tree. When running with 2.1.0~rc2-1, I started getting errors:

 12. from bzrlib.workingtree import WorkingTree
File "/usr/lib/python2.6/dist-packages/bzrlib/workingtree.py" in <module>
  86. from bzrlib.lockable_files import LockableFiles
File "/usr/lib/python2.6/dist-packages/bzrlib/lockable_files.py" in <module>
  64. class LockableFiles(object):
File "/usr/lib/python2.6/dist-packages/bzrlib/lockable_files.py" in LockableFiles
  223. @only_raises(errors.LockNotHeld, errors.LockBroken)
File "/usr/lib/python2.6/dist-packages/bzrlib/lazy_import.py" in __getattribute__
  106. obj = _replace()
File "/usr/lib/python2.6/dist-packages/bzrlib/lazy_import.py" in _replace
  89. obj = factory(self, scope, name)
File "/usr/lib/python2.6/dist-packages/bzrlib/lazy_import.py" in _import
  192. module = __import__(module_python_path, scope, scope, [member])
File "/usr/lib/python2.6/dist-packages/bzrlib/errors.py" in <module>
  20. from bzrlib import (
File "/usr/lib/python2.6/dist-packages/bzrlib/osutils.py" in <module>
  1446. signal.signal(signal.SIGWINCH, _terminal_size_changed)

Exception Type: ValueError at /
Exception Value: signal only works in main thread

This seems to be due to bzrlib/osutils.py now making use of signal.signal. I can probably work around this in my code so that I no longer import WorkingTree, but it seems like this could also cause a problem for systems like loggerhead, so I thought I would go ahead and report the bug. I don't know how the bzr code is factored, if it indeed is supposed to be possible to use WorkingTree inside python threads maybe there is a UI layer that this signal code could be moved to.

Related branches

Martin Pool (mbp) wrote :

We shouldn't be doing this unconditionally or at load time, only (probably) when a TextUIFactory is constructed.

Changed in bzr:
importance: Undecided → Medium
status: New → Confirmed
Martin Pool (mbp) wrote :

Patches to change this gratefully accepted.

Robert Collins (lifeless) wrote :

we'd still want to only do it once, other than that I agree that there is no need to do it on import.

Elliot Murphy (statik) wrote :

I've got tests running on a patch now.

Changed in ubuntuone-servers:
status: New → In Progress
Elliot Murphy (statik) wrote :

Branch proposed for merging.

Elliot Murphy (statik) wrote :

I see that bzr 2.1 final has been released without this bugfix. Developers working on Ubuntu One will need to run a patched version of bzr in order to be able to work on our code (we are all using Lucid), and when we upgrade the production servers to Lucid we will run into this bug there as well.

Could this patch be included in the bzr 2.1 package that gets uploaded to lucid?

Changed in ubuntuone-servers:
status: In Progress → Fix Committed
Martin Pool (mbp) wrote :

Hi Elliot,

We don't use fixcommitted, just inprogress til it's done.

We will put this into 2.1.1 which is likely to get into lucid before the final release. I added a bugtask for that.

Changed in bzr:
status: Confirmed → In Progress
Elliot Murphy (statik) wrote :

Thanks! I only marked the ubuntuone-servers bugtask fix committed.

dobey (dobey) on 2010-02-23
Changed in ubuntuone-servers:
assignee: nobody → Elliot Murphy (statik)
importance: Undecided → Low
James Teh (jteh) wrote :

This also breaks trac-bzr with tracd. I assume tracd must cause plugins to be imported in a different thread.

Martin Pool (mbp) wrote :

will be in 2.1.1

Martin Pool (mbp) wrote :

will be in 2.2.0b1

Changed in bzr:
milestone: none → 2.2.0b1
status: In Progress → Fix Released
Martin Pool (mbp) wrote :

This has been fixed in https://edge.launchpad.net/bzr/2.1/2.1.1 which I've just released.

Do you want/need us to request 2.1.1 to go into Lucid (which currently has 2.1.0)? There are some other bugs, but nothing really critical.

Martin Pool wrote:
> This has been fixed in https://edge.launchpad.net/bzr/2.1/2.1.1 which
> I've just released.
>
> Do you want/need us to request 2.1.1 to go into Lucid (which currently
> has 2.1.0)? There are some other bugs, but nothing really critical.

I think the bug preventing 2.1.0's bzrlib being called from other
threads is pretty close to critical and certainly worthy of getting into
Lucid.

Ian C.

On 24 March 2010 18:42, Ian Clatworthy <email address hidden> wrote:
> Martin Pool wrote:
>> This has been fixed in https://edge.launchpad.net/bzr/2.1/2.1.1 which
>> I've just released.
>>
>> Do you want/need us to request 2.1.1 to go into Lucid (which currently
>> has 2.1.0)?  There are some other bugs, but nothing really critical.
>
> I think the bug preventing 2.1.0's bzrlib being called from other
> threads is pretty close to critical and certainly worthy of getting into
> Lucid.

Note that you can call it from other threads, as long as the initial
import is done from the main thread. But that might be hard to
arrange in some apps. So I'll nominate it for Lucid.

--
Martin <http://launchpad.net/~mbp/>

Changed in bzr (Ubuntu):
status: New → Confirmed
Martin Pool (mbp) wrote :
Download full text (4.1 KiB)

To get into Lucid now, this needs to go through <https://wiki.ubuntu.com/FreezeExceptionProcess#FeatureFreeze+for+bugfix-only+updates>

What follows is the upstream NEWS changes from 2.1.0 (currently in lucid) to 2.1.1:

=== modified file 'NEWS'
--- NEWS 2010-02-16 16:08:40 +0000
+++ NEWS 2010-03-24 08:01:03 +0000
@@ -5,6 +5,66 @@
 .. contents:: List of Releases
    :depth: 1

+
+bzr 2.1.1
+#########
+
+:2.1.1: 2010-03-24
+
+This is a small bugfix release. Upgrading is recommended for anyone
+running 2.1.0 or earlier.
+
+Bug Fixes
+*********
+
+* Allow syscalls to automatically restart when ``TextUIFactory``'s
+ SIGWINCH handler is invoked, avoiding ``EINTR`` errors during blocking
+ IO, which are often poorly handled by Python's libraries and parts of
+ bzrlib. (Andrew Bennetts, #496813)
+
+* Avoid ``malloc(0)`` in ``patiencediff``, which is non-portable.
+ (Martin Pool, #331095)
+
+* Fix plugin packaging on Windows. (Ian Clatworthy, #524162)
+
+* Fix stub sftp test server to call os.getcwdu().
+ (Vincent Ladeuil, #526211, #526353)
+
+* Fixed CHM generation by moving the NEWS section template into
+ a separate file. (Ian Clatworthy, #524184)
+
+* Merge correctly when this_tree is not a WorkingTree. (Aaron Bentley)
+
+* Register SIGWINCH handler only when creating a ``TextUIFactory``; avoids
+ problems importing bzrlib from a non-main thread.
+ (Elliot Murphy, #521989)
+
+* Standardize the error handling when creating a new ``StaticTuple``
+ (problems will raise TypeError). (Matt Nordhoff, #457979)
+
+* Warn if pyrex is too old to compile the new ``SimpleSet`` and
+ ``StaticTuple`` extensions, rather than having the build fail randomly.
+ (John Arbash Meinel, #449776)
+
+Documentation
+*************
+
+* Added a link to the Desktop Guide. (Ian Clatworthy)
+
+* Added What's New in Bazaar 2.1 document. (Ian Clatworthy)
+
+* Drop Google Analytics from the core docs as they caused problems
+ in the CHM files. (Ian Clatworthy, #502010)
+
+API Changes
+***********
+
+* Added ``bzrlib.osutils.set_signal_handler``, a convenience function that
+ can set a signal handler and call ``signal.siginterrupt(signum,
+ False)`` for it, if the platform and Python version supports it.
+ (Andrew Bennetts, #496813)
+
+
 bzr 2.1.0
 #########

@@ -328,23 +388,55 @@
   (Martin Pool)

-bzr 2.0.5 (not released yet)
-############################
+bzr 2.0.5
+#########

 :Codename:
-:2.0.5:
+:2.0.5: NOT RELEASED YET

 Bug Fixes
 *********

+* Avoid ``malloc(0)`` in ``patiencediff``, which is non-portable.
+ (Martin Pool, #331095)
+
+* Concurrent autopacking is more resilient to already-renamed pack files.
+ If we find that a file we are about to obsolete is already obsoleted, we
+ do not try to rename it, and we leave the file in ``obsolete_packs``.
+ The code is also fault tolerant if a file goes missing, assuming that
+ another process already removed the file.
+ (John Arbash Meinel, Gareth White, #507557)
+
+* Cope with the lockdir ``held/info`` file being empty, which seems to
+ happen fairly often if the process is suddenly interrupted while taking
+ a lock.
+ (Martin Pool, #185103)
+
+* Give the warning about potentially slow cross...

Read more...

Martin Pool (mbp) on 2010-03-25
Changed in bzr:
importance: Medium → High
assignee: nobody → Martin Pool (mbp)
Martin Pool (mbp) wrote :

Here is the diff for review. It has some bulk updates to the copyright headers but is pretty easily reviewable.

Elliot Murphy (statik) wrote :

Hi Ubuntu release team, I think this bugfix release is worthy of going into Lucid immediately. This bug breaks Ubuntu One servers, trac-bzr, and I suspect it may break loggerhead as well.

I've briefly reviewed the other bugfixes included in this micro release, and see nothing that should block this from going into Lucid before beta2.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package bzr - 2.2.0-1

---------------
bzr (2.2.0-1) unstable; urgency=low

  * New upstream release.
   + Adds support for setting timestamps to originating revisions.
     Closes: #473450
   + Removes remaining string exception. Closes: #585193, LP: #586926
   + Add C extension to work around Python issue 1628205. LP: #583941,
     Closes: #577110
   + Avoids showing progress bars when --quiet is used. Closes: #542105,
     LP: #320035
   + No longer creates ~/.bazaar as root when run under sudo. LP: #376388
   + 'bzr commit' now supports -p as alternative for --show-diff. LP: #571467
   + 'bzr add' no longer adds .THIS/.BASE/.THEIRS files unless
     explicitly requested. LP: #322767
   + When parsing patch files, Bazaar now supports diff lines before each
     patch. LP: #502076
   + WorkingTrees now no longer requires using signal.signal, so can
     be used in a threaded environment. LP: #521989
   + An assertion error is no longer triggered when pushing to a pre-1.6
     Bazaar server. LP: #528041
  * Bump standards version to 3.9.1.

bzr (2.2.0~b4-1) experimental; urgency=low

  * New upstream beta.
  * Bump standards version to 3.9.0.
  * Drop build dependency on zlib.

bzr (2.2.0~b2-1) experimental; urgency=low

  * New upstream release.
  * Recommend python-launchpadlib. Closes: #568937

bzr (2.1.2-1.1) unstable; urgency=low

  * Non-maintainer upload.
  * Fix access via http_proxy. Closes: #588430
 -- Jelmer Vernooij <email address hidden> Sat, 07 Aug 2010 00:54:52 +0200

Changed in bzr (Ubuntu):
status: Confirmed → Fix Released
Elliot Murphy (statik) on 2010-08-16
Changed in ubuntuone-servers:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers