Merge lp:~mwhudson/loggerhead/no-transport-sharing into lp:loggerhead

[Michael Hudson-Doyle]

[Michael Hudson-Doyle (mwhudson)]

Hi,

This branch was inspired by the bug where we ended up sharing a transport's connection between threads (bug 390972) but I ended up fixing a bunch of other things too:

 * interpreting the 'http_serve' config value more liberally
 * generally, distinguish between .bzr and non-.bzr requests after traversing up to the .bzr data rather than right at the start
 * making serving .bzr directories when --allow-writes is not passed work again
 * check http_serve when serving non-branch .bzr data

Cheers,
mwh

[Matt Nordhoff (mnordhoff)]

"it seems ok to me, but you should really ask beuno". And I didn't actually test it. (I...probably will later.) But anyway... :P

Just a few unimportant comments and questions:

> 49 + def app_for_bazaar_data(self, relpath):

should have the TODO comment from the old function:

> 120 - # TODO: Use something here that uses the transport API
> 121 - # rather than relying on the local filesystem API.

> 69 + elif _bools.get(value.lower(), True):
> 70 + raise httpexceptions.HTTPNotFound()

Like I said before, I think you've got the logic here reversed.

> 78 + self.check_serveable(LocationConfig(self.transport.base))

Will LocationConfig explode horribly if something goes wrong? Are there any exceptions that should be caught?

> 211 + load_plugins()

load_plugins() is already called in main().

[Matt Nordhoff (mnordhoff)]

> > 78 + self.check_serveable(LocationConfig(self.transport.base))
>
> Will LocationConfig explode horribly if something goes wrong? Are there any
> exceptions that should be caught?

Hmm, as it is, LocationConfig will wind up opening another transport, won't it? Is that a problem?

[Matt Nordhoff (mnordhoff)]

Oh, NEWS should be updated for all the bugs this fixes. (I'm linking them against this branch right now. There probably aren't any bugs about the http_serve config value thing, but maybe it should be mentioned too.)

[Michael Hudson-Doyle (mwhudson)]

Matt Nordhoff wrote:
>>> 78 + self.check_serveable(LocationConfig(self.transport.base))
>> Will LocationConfig explode horribly if something goes wrong? Are there any
>> exceptions that should be caught?
>
> Hmm, as it is, LocationConfig will wind up opening another transport, won't it?

I don't think so.

> Is that a problem?

No :)

Cheers,
mwh

[Michael Hudson-Doyle (mwhudson)]

Matt Nordhoff wrote:
> "it seems ok to me, but you should really ask beuno". And I didn't actually test it. (I...probably will later.) But anyway... :P

:)

> Just a few unimportant comments and questions:
>
>> 49 + def app_for_bazaar_data(self, relpath):
>
> should have the TODO comment from the old function:
>
>> 120 - # TODO: Use something here that uses the transport API
>> 121 - # rather than relying on the local filesystem API.

Added.

>> 69 + elif _bools.get(value.lower(), True):
>> 70 + raise httpexceptions.HTTPNotFound()
>
> Like I said before, I think you've got the logic here reversed.

Fixed.

>> 78 + self.check_serveable(LocationConfig(self.transport.base))
>
> Will LocationConfig explode horribly if something goes wrong? Are there any exceptions that should be caught?

As discussed on IRC, I don't think I care very much...

>> 211 + load_plugins()
>
> load_plugins() is already called in main().

Oh yeah! This was bad conflict resolution, I think.

Thanks for the comments.

[Matt Nordhoff (mnordhoff)]

Michael Hudson wrote:
> Matt Nordhoff wrote:
>>>> 78 + self.check_serveable(LocationConfig(self.transport.base))
>>> Will LocationConfig explode horribly if something goes wrong? Are there any
>>> exceptions that should be caught?
>> Hmm, as it is, LocationConfig will wind up opening another transport, won't it?
>
> I don't think so.
>
>> Is that a problem?
>
> No :)
>
> Cheers,
> mwh

OK. :-D
--

[lp:~mwhudson/loggerhead/no-transport-sharing]

381. By Michael Hudson-Doyle on 2009-06-25

mismerge

382. By Michael Hudson-Doyle on 2009-06-25

may as well delay getting a transport a bit

383. By Michael Hudson-Doyle on 2009-06-25

restore TODO

384. By Michael Hudson-Doyle on 2009-06-25

some NEWS

=== modified file 'loggerhead/apps/transport.py'
--- loggerhead/apps/transport.py	2009-06-24 04:29:10 +0000
+++ loggerhead/apps/transport.py	2009-06-25 00:47:19 +0000
@@ -16,8 +16,11 @@
 #
 """Serve branches at urls that mimic a transport's file system layout."""
 
+import threading
+
 from bzrlib import branch, errors, lru_cache, urlutils
-from bzrlib.bzrdir import BzrDir
+from bzrlib.config import LocationConfig
+from bzrlib.transport import get_transport
 from bzrlib.transport.http import wsgi
 
 from paste.request import path_info_pop
@@ -26,9 +29,14 @@
 
 from loggerhead.apps.branch import BranchWSGIApp
 from loggerhead.apps import favicon_app, static_app
-from loggerhead.config import LoggerheadConfig
 from loggerhead.controllers.directory_ui import DirectoryUI
 
+_bools = {
+    'yes': True, 'no': False,
+    'on': True, 'off': False,
+    '1': True, '0': False,
+    'true': True, 'false': False,
+    }
 
 class BranchesFromTransportServer(object):
 
@@ -62,9 +70,8 @@
                 name = self.name
             else:
                 name = '/'
-            return DirectoryUI(environ['loggerhead.static.url'],
-                               self.transport,
-                               name)
+            return DirectoryUI(
+                environ['loggerhead.static.url'], self.transport, name)
         else:
             new_transport = self.transport.clone(segment)
             if self.name:
@@ -73,53 +80,70 @@
                 new_name = '/' + segment
             return BranchesFromTransportServer(new_transport, self.root, new_name)
 
+    def app_for_bazaar_data(self, relpath):
+        if relpath == '/.bzr/smart':
+            wsgi_app = wsgi.SmartWSGIApp(self.transport)
+            return wsgi.RelpathSetter(wsgi_app, '', 'PATH_INFO')
+        else:
+            base = self.transport.base
+            readonly_prefix = 'readonly+'
+            if base.startswith(readonly_prefix):
+                base = base[len(readonly_prefix):]
+            try:
+                path = urlutils.local_path_from_url(base)
+            except errors.InvalidURL, e:
+                raise httpexceptions.HTTPNotFound()
+            else:
+                return urlparser.make_static(None, path)
+
+    def check_serveable(self, config):
+        value = config.get_user_option('http_serve')
+        if value is None:
+            return
+        elif _bools.get(value.lower(), True):
+            raise httpexceptions.HTTPNotFound()
+
     def __call__(self, environ, start_response):
+        path = environ['PATH_INFO']
         try:
             b = branch.Branch.open_from_transport(self.transport)
         except errors.NotBranchError:
+            if path.startswith('/.bzr'):
+                self.check_serveable(LocationConfig(self.transport.base))
+                return self.app_for_bazaar_data(path)(environ, start_response)
             if not self.transport.listable() or not self.transport.has('.'):
                 raise httpexceptions.HTTPNotFound()
             return self.app_for_non_branch(environ)(environ, start_response)
         else:
-            if b.get_config().get_user_option('http_serve') == 'False':
-                raise httpexceptions.HTTPNotFound()
+            self.check_serveable(b.get_config())
+            if path.startswith('/.bzr'):
+                return self.app_for_bazaar_data(path)(environ, start_response)
             else:
                 return self.app_for_branch(b)(environ, start_response)
 
 
+_transport_store = threading.local()
+
+def get_transport_for_thread(base):
+    """ """
+    thread_transports = getattr(_transport_store, 'transports', None)
+    if thread_transports is None:
+        thread_transports = _transport_store.transports = {}
+    if base in thread_transports:
+        return thread_transports[base]
+    transport = get_transport(base)
+    return transport
+
+
 class BranchesFromTransportRoot(object):
 
-    def __init__(self, transport, config):
+    def __init__(self, base, config):
         self.graph_cache = lru_cache.LRUCache(10)
-        self.transport = transport
-        wsgi_app = wsgi.SmartWSGIApp(self.transport)
-        self.smart_server_app = wsgi.RelpathSetter(wsgi_app, '', 'PATH_INFO')
+        self.base = base
         self._config = config
 
-    def get_local_path(self):
-        """Raise exception if it's not a local path, otherwise return it"""
-
-        # TODO: Use something here that uses the transport API 
-        # rather than relying on the local filesystem API.
-        try:
-            path = urlutils.local_path_from_url(self.transport.base)
-        except errors.InvalidURL:
-            raise httpexceptions.HTTPNotFound()
-        else:
-            return path
-
-    def check_is_a_branch(self, path_info):
-        """Check if it's a branch, and that it's allowed to be shown"""
-        try:
-            bzrdir = BzrDir.open_containing_from_transport(
-                       self.transport.clone(path_info))[0]
-            branch = bzrdir.open_branch()
-            if branch.get_config().get_user_option('http_serve') == 'False':
-                raise httpexceptions.HTTPNotFound()
-        except errors.NotBranchError:
-            return
-
     def __call__(self, environ, start_response):
+        transport = get_transport_for_thread(self.base)
         environ['loggerhead.static.url'] = environ['SCRIPT_NAME']
         if environ['PATH_INFO'].startswith('/static/'):
             segment = path_info_pop(environ)
@@ -127,30 +151,23 @@
             return static_app(environ, start_response)
         elif environ['PATH_INFO'] == '/favicon.ico':
             return favicon_app(environ, start_response)
-        elif environ['PATH_INFO'].endswith("/.bzr/smart"):
-            self.check_is_a_branch(environ['PATH_INFO'])
-            return self.smart_server_app(environ, start_response)
-        elif '/.bzr/' in environ['PATH_INFO']:
-            self.check_is_a_branch(environ['PATH_INFO'])
-            path = self.get_local_path()
-            app = urlparser.make_static(None, path)
-            return app(environ, start_response)
         else:
             return BranchesFromTransportServer(
-                self.transport, self)(environ, start_response)
+                transport, self)(environ, start_response)
 
 
 class UserBranchesFromTransportRoot(object):
 
-    def __init__(self, transport, config):
+    def __init__(self, base, config):
         self.graph_cache = lru_cache.LRUCache(10)
-        self.transport = transport
+        self.base = base
         self._config = config
         self.trunk_dir = config.get_option('trunk_dir')
 
     def __call__(self, environ, start_response):
         environ['loggerhead.static.url'] = environ['SCRIPT_NAME']
         path_info = environ['PATH_INFO']
+        transport = get_transport_for_thread(self.base)
         if path_info.startswith('/static/'):
             segment = path_info_pop(environ)
             assert segment == 'static'
@@ -161,10 +178,11 @@
             # segments starting with ~ are user branches
             if path_info.startswith('/~'):
                 segment = path_info_pop(environ)
-                new_transport = self.transport.clone(segment[1:])
+                new_transport = transport.clone(segment[1:])
                 return BranchesFromTransportServer(
-                    new_transport, self, segment)(environ, start_response)
+                    transport.clone(segment[1:]), self, segment)(
+                    environ, start_response)
             else:
-                new_transport = self.transport.clone(self.trunk_dir)
                 return BranchesFromTransportServer(
-                    new_transport, self)(environ, start_response)
+                    transport.clone(self.trunk_dir), self)(
+                    environ, start_response)

=== modified file 'loggerhead/main.py'
--- loggerhead/main.py	2009-06-24 05:11:14 +0000
+++ loggerhead/main.py	2009-06-25 00:50:40 +0000
@@ -48,10 +48,16 @@
         config.print_help()
         sys.exit(1)
     elif config.arg_count == 1:
-        path = config.get_arg(0)
+        base = config.get_arg(0)
     else:
-        path = '.'
-    return config, path
+        base = '.'
+
+    load_plugins()
+
+    if not config.get_option('allow_writes'):
+        base = 'readonly+' + base
+
+    return config, base
 
 
 def setup_logging(config):
@@ -72,12 +78,7 @@
     return logger
 
 
-def make_app_for_config_and_path(config, path):
-    if config.get_option('allow_writes'):
-        transport = get_transport(path)
-    else:
-        transport = get_transport('readonly+' + path)
-
+def make_app_for_config_and_path(config, base):
     if config.get_option('trunk_dir') and not config.get_option('user_dirs'):
         print "--trunk-dir is only valid with --user-dirs"
         sys.exit(1)
@@ -92,9 +93,9 @@
         if not config.get_option('trunk_dir'):
             print "You didn't specify a directory for the trunk directories."
             sys.exit(1)
-        app = UserBranchesFromTransportRoot(transport, config)
+        app = UserBranchesFromTransportRoot(base, config)
     else:
-        app = BranchesFromTransportRoot(transport, config)
+        app = BranchesFromTransportRoot(base, config)
 
     setup_logging(config)