launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Pushed some changes, and I would like some more feedbacks in a couple of questions replied below.

Diff comments:

> diff --git a/lib/lp/scripts/runlaunchpad.py b/lib/lp/scripts/runlaunchpad.py
> index 0557ee8..79ba3d1 100644
> --- a/lib/lp/scripts/runlaunchpad.py
> +++ b/lib/lp/scripts/runlaunchpad.py
> @@ -332,7 +334,31 @@ def start_testapp(argv=list(sys.argv)):
>                  pass
>  
>  
> +def gunicornify_zope_config_file():
> +    """Creates a new launchpad.config file removing directives related to
> +    Zope Server that shouldn't be used when running on gunicorn.
> +    """
> +    original_filename = config.zope_config_file
> +    with open(original_filename) as fd:
> +        content = fd.read()
> +
> +    # Remove unwanted tags.
> +    for tag in ['server', 'accesslog']:
> +        content = re.sub(
> +            r"<%s>(.*)</%s>" % (tag, tag), "", content, flags=re.S)
> +
> +    # Remove unwanted single-line directives.
> +    for directive in ['interrupt-check-interval']:
> +        content = re.sub(r"%s .*" % directive, "", content)
> +
> +    new_filename = tempfile.mktemp(prefix='launchpad.conf-gunicorn-')
> +    with open(new_filename, 'w') as fd:
> +        fd.write(content)
> +    config.zope_config_file = new_filename

That for sure works.

I'll change LaunchpadConfig._getZConfig to mimic zope.app.wsgi.config's behavior, but it is not strictly necessary since LaunchpadConfig will not get any configuration from the Zope config file anymore.

> +
> +
>  def gunicorn_main():
> +    gunicornify_zope_config_file()
>      orig_argv = sys.argv
>      try:
>          sys.argv = [
> diff --git a/lib/lp/services/webapp/publication.py b/lib/lp/services/webapp/publication.py
> index 7325d9f..b245c92 100644
> --- a/lib/lp/services/webapp/publication.py
> +++ b/lib/lp/services/webapp/publication.py
> @@ -7,6 +7,7 @@ __all__ = [
>      'LaunchpadBrowserPublication',
>      ]
>  
> +import logging

Right! Leftover from some experiment. Removing it.

>  import re
>  import sys
>  import threading
> @@ -871,6 +872,8 @@ def tracelog(request, prefix, msg):
>      easier. ``prefix`` should be unique and contain no spaces, and
>      preferably a single character to save space.
>      """
> -    tracelog = ITraceLog(request, None)
> -    if tracelog is not None:
> -        tracelog.log('%s %s' % (prefix, six.ensure_str(msg, 'US-ASCII')))
> +    if not config.use_gunicorn:
> +        msg = '%s %s' % (prefix, six.ensure_str(msg, 'US-ASCII'))
> +        tracelog = ITraceLog(request, None)
> +        if tracelog is not None:
> +            tracelog.log(msg)

It seems that zc.zservertracelog is quite attached to Zope Server, and I couldn't find a way to use it with WSGI/gunicorn.

For example, when we try to adapt request using ITraceLog, we get a KeyError on 'zc.zservertracelog.interfaces.ITraceLog' because zc.zservertracelog tries to get this key from the request, but it's only set on zc.zservertracelog.tracelog.Server (which is used  to create a server type a bit below in the code).

I might be missing something here, but maybe we need another way to log this. Maybe simply `logging.getLoger().debug(msg)` directly here when we are using gunicorn, instead of ignoring the message?

> diff --git a/lib/lp/testing/layers.py b/lib/lp/testing/layers.py
> index 9089c60..98c8149 100644
> --- a/lib/lp/testing/layers.py
> +++ b/lib/lp/testing/layers.py
> @@ -155,7 +160,6 @@ from lp.testing import (
>      reset_logging,
>      )
>  from lp.testing.pgsql import PgTestSetup
> -import zcml

Good catch. I'm changing format-imports script.

>  
>  
>  COMMA = ','
> @@ -1794,6 +1798,14 @@ class LayerProcessController:
>              raise LayerIsolationError(
>                  "App server died in this test (status=%s):\n%s" % (
>                      cls.appserver.returncode, cls.appserver.stdout.read()))
> +        # Cleanup the app server's output buffer between tests.
> +        while True:
> +            # Read while we have something available at the stdout.
> +            r, w, e = select.select([cls.appserver.stdout], [], [], 0)
> +            if cls.appserver.stdout in r:
> +                cls.appserver.stdout.readline()

It's odd, but line breaks actually help a lot here.

If I just use `read`, the call would block forever, since the server is not being shut down (so, EOF will never be reached). 

Calling `read(limit)` wouldn't help much either, since we have no idea on the size of the output so far, and to overcome this we would need to select/read byte-per-byte. Might be too much, since we know that the output is a plain-text log, separated by line breaks anyway.

> +            else:
> +                break
>          DatabaseLayer.force_dirty_database()
>  
>      @classmethod


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/396691
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:gunicorn-appserver-tests.