1201113 - Support to run Bugzilla as a PSGI application

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Frédéric Buclin]

Attachment: patch, v1

While playing with PSGI support for Bugzilla, I realized that I was very close from what mkanat did in bug 316665. I looked if we could simply use CGI::Compile directly, but it appeared it was easier to use Plack directly as it handles several things for us for free. Also, we already indirectly depend on Plack since JSON::RPC decided to depend on it and so Plack is not a totally new dependency. So I merged the best of the two works, excluding stuff related to FastCGI and cleanup which I considered as not needed.

I used Starman on Linux to test this patch, and everything is working fine.

Comment 1

[David Lawrence [:dkl]]

Comment on attachment 8656037 [details] [diff] [review]
patch, v1

I will defer this one to Dylan. My plate is full right now.

Comment 2

[Frédéric Buclin]

Attachment: patch, v1.1

editkeywords.cgi and editgroups.cgi are duplicating calls to $cgi->header(). This is incorrect and must be fixed to work correctly with PSGI.

Comment 3

[Frédéric Buclin]

Attachment: patch, v1.2

My previous patch didn't play with shutdownhtml nicely, because you must exit from the PSGI application itself. So I created shutdown.cgi to handle this correctly, and the wrapper will call it if shutdownhtml is set, unless the script being called is editparams.cgi as this one is whitelisted (to reset the parameter).

With this patch, all QA Selenium scripts pass against a patched 5.0.1 installation. I tested against 5.0.1 because QA tests are known to all pass there. The performance win is very significative. On average, using nginx + gazelle (http://search.cpan.org/~kazeburo/Gazelle/, which is faster than starman) makes tests to run almost 4 times faster than apache + mod_cgi.

QA Selenium scripts               nginx + gazelle     mod_cgi  ratio
====================================================================
test_bug_edit.t .................. ok    41889 ms   158481 ms   3.78
test_choose_priority.t ........... ok     6324 ms    17913 ms   2.83
test_classifications.t ........... ok    10404 ms    35092 ms   3.37
test_config.t .................... ok     5979 ms    18058 ms   3.02
test_create_user_accounts.t ...... ok    11215 ms    52664 ms   4.70
test_custom_fields_admin.t ....... ok    27137 ms    54719 ms   2.02
test_custom_fields.t ............. ok    42433 ms   196001 ms   4.62
test_default_groups.t ............ ok    18763 ms    94797 ms   5.05
test_dependencies.t .............. ok     9751 ms    35844 ms   3.68
test_edit_products_properties.t .. ok    20861 ms    98284 ms   4.71
test_email_preferences.t ......... ok    24001 ms    88555 ms   3.69
test_enter_new_bug.t ............. ok     8582 ms    30591 ms   3.56
test_flags2.t .................... ok    23564 ms   100350 ms   4.26
test_flags.t ..................... ok    32709 ms   129874 ms   3.97
test_groups.t .................... ok    34694 ms   149843 ms   4.32
test_keywords.t .................. ok    17747 ms    56581 ms   3.19
test_login.t ..................... ok     4367 ms     7494 ms   1.72
test_milestones.t ................ ok    20104 ms    73154 ms   3.64
test_password_complexity.t ....... ok    28237 ms   115517 ms   4.09
test_private_attachments.t ....... ok    23627 ms    85891 ms   3.64
test_qa_contact.t ................ ok    19246 ms    67614 ms   3.51
test_require_login.t ............. ok    16128 ms    79844 ms   4.95
test_sanity_check.t .............. ok    10888 ms    35622 ms   3.27
test_saved_searches.t ............ ok    12235 ms    42633 ms   3.48
test_search.t .................... ok     9641 ms    25855 ms   2.68
test_security.t .................. ok    29260 ms   113768 ms   3.89
test_shared_searches.t ........... ok    18662 ms    75953 ms   4.07
test_show_all_products.t ......... ok     7258 ms    23758 ms   3.27
test_shutdown.t .................. ok    14673 ms    76121 ms   5.19
test_status_whiteboard.t ......... ok    19922 ms    68483 ms   3.44
test_strict_isolation.t .......... ok    19522 ms    71491 ms   3.66
test_sudo_sessions.t ............. ok    11476 ms    43088 ms   3.75
test_target_milestones.t ......... ok    17377 ms    63839 ms   3.67
test_time_summary.t .............. ok    12571 ms    39261 ms   3.12
test_user_groups.t ............... ok    19514 ms    84057 ms   4.31
test_user_matching.t ............. ok    21092 ms    91927 ms   4.36
test_user_preferences.t .......... ok    26006 ms   107370 ms   4.13
test_user_privs.t ................ ok     6996 ms    22371 ms   3.20
test_votes.t ..................... ok     3664 ms     3895 ms   1.06

wallclock secs                        708 (11'48) 2737 (45'37)  3.87


I also tested QA WebServices scripts, and XML-RPC doesn't play nicely with Plack. It looks like a module like SOAP::Transport::HTTP::Plack (http://search.cpan.org/~helena/SOAP-Transport-HTTP-Plack/) is required to make it work, but I don't plan to fix it in this bug as the XML-RPC API is marked as deprecated since Bugzilla 5.0 and it doesn't look like to be a trivial fix. But the JSON-RPC and REST API are working fine.

To ignore XML-RPC in tests below, I simply patched QA::Util::get_rpc_clients() to return $jsonrpc instead of $xmlrpc (meaning that JSON-RPC is tested twice). The performance win is huge. For instance, webservice_bug_fields.t runs 88 times faster!! On average, tests run 19 times faster.

QA WebServices scripts (JSON-RPC only)    nginx + gazelle     mod_cgi  ratio
============================================================================
webservice_bug_add_attachment.t ........... ok    8268 ms    66838 ms    8.1
webservice_bug_add_comment.t .............. ok    4665 ms    55003 ms   11.8
webservice_bug_attachments.t .............. ok    4215 ms    68073 ms   16.2
webservice_bug_comments.t ................. ok    5121 ms    73807 ms   14.4
webservice_bug_create.t ................... ok    6888 ms    82097 ms   11.9
webservice_bug_fields.t ................... ok    4249 ms   376125 ms   88.5
webservice_bug_get.t ...................... ok    4772 ms    81761 ms   17.1
webservice_bug_history.t .................. ok    2306 ms    54484 ms   23.6
webservice_bug_legal_values.t ............. ok    3711 ms   138567 ms   37.3
webservice_bug_search.t ................... ok    9550 ms   162179 ms   17.0
webservice_bug_update_see_also.t .......... ok    5115 ms    73590 ms   14.4
webservice_bug_update.t ................... ok   34523 ms   451806 ms   13.1
webservice_bugzilla.t ..................... ok     306 ms    11684 ms   38.2
webservice_group_create.t ................. ok    2229 ms    34262 ms   15.4
webservice_jsonp.t ........................ ok     475 ms    30389 ms   64.0
webservice_product_create.t ............... ok    4028 ms    52511 ms   13.0
webservice_product_get.t .................. ok    2123 ms    53960 ms   25.4
webservice_user_create.t .................. ok    2372 ms    35131 ms   14.8
webservice_user_get.t ..................... ok    3419 ms   109307 ms   32.0
webservice_user_login_logout.t ............ ok    1630 ms    52984 ms   32.5
webservice_user_offer_account_by_email.t .. ok     473 ms    12148 ms   25.7

wallclock secs                                 110 (1'50) 2077 (34'37)  18.9

Comment 4

[Frédéric Buclin]

Attachment: backport for 5.0.1

As I tested my code against 5.0.1, I attach my patch for 5.0.1 here. It's exactly the same patch as for master, except it fixes a minor conflict in .htaccess. But this backport is not for review as there is no plan to add support for PSGI in the 5.0 branch. Have fun!

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8666452 [details] [diff] [review]
patch, v1.2

Review of attachment 8666452 [details] [diff] [review]:
-----------------------------------------------------------------

r-

fix these small issues. Have you tested xmlrpc/jsonrpc against this?

::: Bugzilla.pm
@@ +16,5 @@
> +    # This makes sure we're in a CGI. mod_perl doesn't support Carp
> +    # and Plack reports errors elsewhere.
> +    # We cannot call i_am_persistent() from here as its module is
> +    # not loaded yet.
> +    if ($ENV{SERVER_SOFTWARE} && !($ENV{MOD_PERL} || $ENV{PLACK_ENV})) {

PLACK_ENV specifies the type of environment, but I don't feel comfortable relying on it to mean "we're running under the psgi".
it's better to just explicitly do that in app.psgi

::: Bugzilla/Install/Filesystem.pm
@@ +165,4 @@
>          'whine.pl'        => { perms => WS_EXECUTE },
>          'email_in.pl'     => { perms => WS_EXECUTE },
>          'sanitycheck.pl'  => { perms => WS_EXECUTE },
> +        'app.psgi'        => { perms => OWNER_EXECUTE | WS_SERVE },

psgi files are neither executable nor serveable. They read read and eval'd by a server process.

::: Bugzilla/Install/Util.pm
@@ +84,5 @@
>               os_ver   => $os_details[3] };
>  }
>  
> +sub i_am_persistent {
> +    return ($ENV{MOD_PERL} || $ENV{PLACK_ENV}) ? 1 : 0;

I don't think you should use $ENV{PLACK_ENV} for this. Set a global in app.psgi and check that.

::: app.psgi
@@ +43,5 @@
> +    my $shutdown_app = Plack::App::WrapCGI->new(script => 'shutdown.cgi')->to_app;
> +
> +    foreach my $cgi_script (@cgis) {
> +        my $app = eval { Plack::App::WrapCGI->new(script => $cgi_script)->to_app };
> +        next if $@;

don't silently swallow errors here. if ($@) { warn "$@"; next }

Comment 6

[Dylan Hardison [:dylan] (he/him)]

(In reply to Dylan William Hardison [:dylan] from comment #5)

> fix these small issues. Have you tested xmlrpc/jsonrpc against this?
I mean just xmlrpc. I never make use of it but I'll test it in review if we're not sure it works.

Comment 7

[Frédéric Buclin]

Attachment: patch, v2

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8692064 [details] [diff] [review]
patch, v2

Review of attachment 8692064 [details] [diff] [review]:
-----------------------------------------------------------------

Can't r+ yet as it seems to fail under perl 5.18.
Actually, the patch doesn't fail -- master does not pass tests on a fresh 5.18 install.

not ok 116 - use Bugzilla::API::Server;
#   Failed test 'use Bugzilla::API::Server;'
#   at t/001compile.t line 39.
#     Tried to use 'Bugzilla::API::Server'.
#     Error:  Invalid default '' for Bugzilla::API::Server->controller not a coderef or a non-ref or code-convertible object at /System/Library/Perl/Extras/5.18/Method/Generate/Accessor.pm line 636.

Comment 9

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #8)
> #     Error:  Invalid default '' for Bugzilla::API::Server->controller

Is it really legal to define:

  has controller => (is => 'rw', default => undef);

If it's undef by default, why defining it at all?

Comment 10

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8692064 [details] [diff] [review]
patch, v2

Review of attachment 8692064 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

Comment 11

[Frédéric Buclin]

Yay! Thanks dylan! :)

To ssh://[e-mail]/bugzilla/bugzilla.git
   c94abb0..8bbc156  master -> master