475086 - (bz-rpc-any) Move away from SOAP::Lite (to RPC::Any)

Status: RESOLVED WONTFIX

(Bugzilla :: WebService, enhancement)

Description

[Max Kanat-Alexander]

SOAP::Lite is really a SOAP package with XML-RPC support hacked into it. It causes all sorts of development problems, in particular it's very hard to extend it (we have to do lots of unsafe digging around with its internal variables, or sometimes we just straight-up *can't* do something).

I did some prototyping of our WebServices interface using different modules from CPAN, and I discovered that RPC::XML is a much better framework than SOAP::Lite for what we need. However, RPC::XML has two problems that have to be fixed before we can use it:

  It doesn't have a good mod_cgi server by default:
  http://rt.cpan.org/Ticket/Display.html?id=42736

  It uses error codes that overlap with ours:
  http://rt.cpan.org/Public/Bug/Display.html?id=42736

Comment 1

[Max Kanat-Alexander]

(In reply to comment #0)
>   It doesn't have a good mod_cgi server by default:
>   http://rt.cpan.org/Ticket/Display.html?id=42736

  Oops. That should have been:
  http://rt.cpan.org/Public/Bug/Display.html?id=42298

Comment 2

[Max Kanat-Alexander]

Note that using RPC::XML will also get us system.multicall functionality for free (along with various other built-in functions).

Comment 3

[Marc Schumann [:Wurblzap]]

If it's helpful, we should do it.

I'd be happy to see this help on bug 352879, too. While XMLRPC is nice, WSDL-backed SOAP support would open doors. At least, we shouldn't move backwards in this regard.

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> I'd be happy to see this help on bug 352879, too. While XMLRPC is nice,
> WSDL-backed SOAP support would open doors. At least, we shouldn't move
> backwards in this regard.

  As far as that goes, the two best modules currently available are SOAP::WSDL and XML::Compile::SOAP.

  However, I'm not really sure what we need SOAP for. XML-RPC currently does everything we need (well, except for dates with timezones). I think we couldn't concurrently maintain a SOAP interface and a JSON-RPC interface, since the protocols are too different, while we could maintain an XML-RPC and JSON-RPC interface simultaneously. I could be wrong, though.

Comment 5

[Max Kanat-Alexander]

Attachment: Hacky hack work in progress

Here's some experimental work in progress to see if RPC::XML is better than SOAP::Lite. So far it is.

Comment 6

[David Lawrence [:dkl]]

(In reply to comment #4)
> (In reply to comment #3)
> > I'd be happy to see this help on bug 352879, too. While XMLRPC is nice,
> > WSDL-backed SOAP support would open doors. At least, we shouldn't move
> > backwards in this regard.
> 
>   As far as that goes, the two best modules currently available are SOAP::WSDL
> and XML::Compile::SOAP.
> 
>   However, I'm not really sure what we need SOAP for. XML-RPC currently does
> everything we need (well, except for dates with timezones). I think we couldn't
> concurrently maintain a SOAP interface and a JSON-RPC interface, since the
> protocols are too different, while we could maintain an XML-RPC and JSON-RPC
> interface simultaneously. I could be wrong, though.

I had a request the other day if we could have a SOAP API with WSDL in addition to the XMLRPC API. I thought since we were using SOAP::Lite it should not be too difficult but now we are moving away from that. How difficult would be in your opinion to add SOAP w/WSDL using either SOAP::WSDL or XML::Compile::SOAP as another Server module when this work is complete?

Dave

Comment 7

[David Lawrence [:dkl]]

(In reply to comment #6)
> I had a request the other day if we could have a SOAP API with WSDL in addition
> to the XMLRPC API. I thought since we were using SOAP::Lite it should not be
> too difficult but now we are moving away from that. How difficult would be in
> your opinion to add SOAP w/WSDL using either SOAP::WSDL or XML::Compile::SOAP
> as another Server module when this work is complete?

Also the reason for the request is that apparently Meta Matrix (http://www.redhat.com/metamatrix/), which we are using now for reporting by combining data from various separate tools, supports SOAP for data conduit but not so much for XMLRPC.

Dave

Comment 8

[Max Kanat-Alexander]

  Hey Dave. Probably best to discuss adding SOAP support in bug 352879.

Comment 9

[Max Kanat-Alexander]

  Okay, I've done a lot of work on this now, and I have come to the conclusion that no matter what module we use, we're going to have to modify it beyond recognition in order to get it working properly. I had to do the same thing for JSON-RPC, and then I had to hack both of them to have compatible APIs.

  So now, basically, I'm considering writing my own RPC server module for Perl, that I'd put on CPAN. I'd probably call it RPC::Any, and it would just implement the simple stuff that we need, exactly how we need it, with a compatible API across RPC methods.

Comment 10

[Max Kanat-Alexander]

  I've made a lot of progress on RPC::Any. Until it's released, the bzr repository is the source of its code:

  bzr://bzr.everythingsolved.com/rpc-any/trunk

  It doesn't have any documentation, it doesn't properly throw exceptions yet, and it doesn't do HTTP yet, but it does XML-RPC, and all three versions of JSON-RPC, if given valid input.

Comment 11

[Max Kanat-Alexander]

Attachment: Work In Progress with RPC::Any

Okay, RPC::Any is pretty much up and running, but I haven't posted it to CPAN yet. It still needs a lot of POD.

To make sure that it's ready for prime time, I've made the XML-RPC interface of Bugzilla work with it! This is a fully-functioning XML-RPC interface, in this patch. As you will see, it is WAY WAY simpler than using SOAP::Lite.

The strip_undefs code is still there because I have to bring that over to RPC::Any, in some way or another.

Comment 12

[Max Kanat-Alexander]

Attachment: v1

Okay, here is a patch that fully converts Bugzilla to using RPC::Any. It retains all of Bugzilla's support, including GET and JSONP support for JSON-RPC.

There are a few changes that are important:

* RPC::Any currently uses RPC::XML as its backend for parsing XML-RPC.
  RPC::XML is very strict about its parsing, so Bugzilla will no longer
  accept blank values for int, double, and dateTime fields.

* RPC::XML also does not accept TZ specifiers on inbound datetimes, so
  Bugzilla will no longer accept those. (It's only accepted them since 3.6
  anyhow.)

* RPC::Any accepts *both* base64-encoded and non-encoded "params" in GET
  requests for JSON-RPC. It figures out which one it's getting, automatically.

RPC::Any is about a zillion times better than either of the modules that we were using before. It retains taint, it has heavily-tested and proper Unicode behavior, it's extremely extendable, and it uses Moose. As you will see, most of this patch involves *removing* code from Bugzilla that is no longer necessary.

RPC::Any hasn't made it to CPAN yet (because I still have to write POD for it), but you can get it using the bzr URL above, and symllink or copy the lib/RPC any directory from RPC::Any into your Bugzilla lib/ directory (which is what I did for testing).

Comment 13

[Frédéric Buclin]

Comment on attachment 451203 [details] [diff] [review]
v1

>=== modified file 'Bugzilla/Install/Requirements.pm'

>+        package => 'RPC-Any',
>+        module  => 'RPC::Any',

Moose and Moose::Role must be listed too, isn't it?

Comment 14

[Max Kanat-Alexander]

(In reply to comment #13)
> Moose and Moose::Role must be listed too, isn't it?

  Not really--they're required by RPC::Any; that's the only reason they're being used. But I could explicitly list Moose, if you want me to.

Comment 15

[Max Kanat-Alexander]

Comment on attachment 451203 [details] [diff] [review]
v1

Hey glob--I know that you were looking at RPC::Any earlier. Do you want to do this review? :-)

Comment 16

[Max Kanat-Alexander]

By the way, if you want, I can split this into three patches: one that modifies the base WebServices framework, then another for XML-RPC, and then another for JSON-RPC. Just let me know if that you would be easier for you.

Comment 17

[:glob ✱]

(In reply to comment #15)
> Hey glob--I know that you were looking at RPC::Any earlier. Do you want to do
> this review? :-)

i'm not familiar enough with bugzilla's rpc mechanisms to feel comfortable reviewing this at this point, i'm sorry.

Comment 18

[Max Kanat-Alexander]

Comment on attachment 451203 [details] [diff] [review]
v1

Okay, no problem.

Comment 19

[Max Kanat-Alexander]

Attachment: v2

Okay, I learned that I can't use has +attr in a Role (it doesn't work in newer versions of Moose). So I moved the setting of dispatch into before 'get_module', which is the part where it's actually needed. I also moved the hook stuff into that function, so that it happens there instead of in a constant, and so that WS_DISPATCH can go back to being a real constant.

Comment 20

[David Lawrence [:dkl]]

(In reply to comment #16)
> By the way, if you want, I can split this into three patches: one that modifies
> the base WebServices framework, then another for XML-RPC, and then another for
> JSON-RPC. Just let me know if that you would be easier for you.

If it is not too much extra trouble, it does make it easier on my end to review if in smaller chunks.

Thanks
Dave

Comment 21

[Max Kanat-Alexander]

Okay, sure. Let me totally finish my testing (I'm making sure all the QA test functions work) and get an RPC::Any 1.00, and then I'll post a patch for you to review.

Comment 22

[Max Kanat-Alexander]

Attachment: Base, v3

Okay, here is the basic modifications to the WebServices framework that aren't protocol-specific.

Comment 23

[Max Kanat-Alexander]

Attachment: XML-RPC, v3

Here are the XML-RPC-specific modifications.

Comment 24

[Max Kanat-Alexander]

Attachment: JSON-RPC, v3

And here are the JSON-RPC-specific changes.

Comment 25

[:glob ✱]

hi max, in bug 470233 you said:

  "currently Moose is gigantic in terms of dependencies and compile time,
  so we can't use it in mod_cgi".

and suggest using mouse instead of moose.

the mouse documentation backs this up, with:

  "Unfortunately, Moose has a compile-time penalty. Though significant
  progress has been made over the years, the compile time penalty is a
  non-starter for some very specific applications. If you are writing a
  command-line application or CGI script where startup time is essential,
  you may not be able to use Moose"

won't bringing in moose via rpc::any impact our mod_cgi performance?

Comment 26

[Max Kanat-Alexander]

glob: It heavily impacts our mod_cgi performance. It's funny that you bring that up, I was going to comment on it last night. My viewpoint is that if people want really speedy WebServices, they should use mod_perl. Since it only affects the WebService and AJAX, and not any of the main user-facing UIs, I think it's acceptable to take the performance hit. It's something that will go in the relnotes, though--that if people want fast WebServices, they should use mod_perl.

Comment 27

[:glob ✱]

(In reply to comment #26)
> Since it only affects the WebService and AJAX, and not any of the main
> user-facing UIs

there's ui code which calls our json interface (such as the duplicate bug finder, and the user auto-complete), and i would expect more of the same in future.

rather than cripple the ajax performance of bugzilla, wouldn't it be better to use a Mouse based module?  maybe RPC::Any could use Any::Moose, and bugzilla could require explicitly Mouse.

Comment 28

[Max Kanat-Alexander]

(In reply to comment #27)
> there's ui code which calls our json interface (such as the duplicate bug
> finder, and the user auto-complete), and i would expect more of the same in
> future.

  That is already pretty slow under mod_cgi.

> rather than cripple the ajax performance of bugzilla, wouldn't it be better to
> use a Mouse based module?  maybe RPC::Any could use Any::Moose, and bugzilla
> could require explicitly Mouse.

  It wouldn't matter--JSON::RPC::Common also uses Moose. More and more of CPAN is using Moose, too--we can't escape it forever.

  The real solution for mod_cgi users would be that I could provide a separate daemon to run RPC::Any, that Bugzilla could defer to, or that could run as its own HTTP instance.

  Or we could provide FastCGI support for Bugzilla, which would also handle the problem, because almost everybody can run a FastCGI process.

Comment 29

[:glob ✱]

(In reply to comment #28)
> Or we could provide FastCGI support for Bugzilla, which would also handle the
> problem, because almost everybody can run a FastCGI process.

that makes sense; even IIS has fastcgi support :)

Comment 30

[Max Kanat-Alexander]

I've released RPC::Any 1.00 to the CPAN, so it should be installable via install-module.pl within a day or two.

Comment 31

[David Lawrence [:dkl]]

Comment on attachment 451842 [details] [diff] [review]
Base, v3

Continuing review once I fixed the patch on my end but this will need to be revised as it is bit-rotted due to the JSON::XS requirement added to Bugzilla/Install/Requirements.pm.

[dkl@localhost trunk]$ patch -p0 < ~/Downloads/rpcany-base.diff 
patching file Bugzilla.pm
patching file Bugzilla/Constants.pm
Hunk #1 succeeded at 143 (offset -1 lines).
patching file Bugzilla/Error.pm
patching file Bugzilla/Install/Requirements.pm
Hunk #1 FAILED at 228.
1 out of 1 hunk FAILED -- saving rejects to file Bugzilla/Install/Requirements.pm.rej
patching file Bugzilla/WebService.pm
patching file Bugzilla/WebService/Constants.pm
patching file Bugzilla/WebService/Server.pm
patching file Bugzilla/WebService/Util.pm
patching file t/002goodperl.t
patching file template/en/default/global/code-error.html.tmpl
Hunk #1 succeeded at 445 (offset -4 lines).

Will JSON::XS be needed anymore once this lands?

Dave

Comment 32

[Max Kanat-Alexander]

(In reply to comment #31)
> Continuing review once I fixed the patch on my end but this will need to be
> revised as it is bit-rotted due to the JSON::XS requirement added to
> Bugzilla/Install/Requirements.pm.

  Okay, will fix.

> Will JSON::XS be needed anymore once this lands?

  Yeah, JSON::RPC::Common still uses JSON.pm in the backend.

Comment 33

[Max Kanat-Alexander]

Attachment: Base, v3.1

Okay, here's the base patch with the bitrot fixed.

Comment 34

[Max Kanat-Alexander]

This isn't going to make 4.0. As a result, we should document that these changes are coming in 4.2 (like support for and defaulting to JSON-RPC 2.0).

Comment 35

[David Lawrence [:dkl]]

Comment on attachment 454353 [details] [diff] [review]
Base, v3.1

Bitrot:

patching file Bugzilla.pm
Hunk #1 succeeded at 469 (offset 20 lines).
patching file Bugzilla/Constants.pm
Hunk #1 FAILED at 143.
Hunk #2 FAILED at 459.
2 out of 2 hunks FAILED -- saving rejects to file Bugzilla/Constants.pm.rej
patching file Bugzilla/Error.pm
Hunk #1 succeeded at 110 (offset 1 line).
Hunk #2 succeeded at 122 (offset 1 line).
patching file Bugzilla/Install/Requirements.pm
Hunk #1 succeeded at 243 (offset 18 lines).
Hunk #2 succeeded at 256 (offset 18 lines).
patching file Bugzilla/WebService.pm
Hunk #2 succeeded at 37 (offset 2 lines).
patching file Bugzilla/WebService/Constants.pm
Hunk #1 succeeded at 131 (offset 9 lines).
Hunk #2 succeeded at 140 (offset 9 lines).
patching file Bugzilla/WebService/Server.pm
patching file Bugzilla/WebService/Util.pm
Hunk #1 FAILED at 23.
Hunk #2 FAILED at 51.
2 out of 2 hunks FAILED -- saving rejects to file Bugzilla/WebService/Util.pm.rej
patching file t/002goodperl.t
patching file template/en/default/global/code-error.html.tmpl
Hunk #1 succeeded at 443 (offset -2 lines).

Comment 36

[Max Kanat-Alexander]

Attachment: Base, v4

Ah, thanks. Here's a patch with the bitrot fixed.

Comment 37

[Max Kanat-Alexander]

Attachment: XML-RPC, v4

Fixes the bitrot for the XML-RPC patch.

Comment 38

[Max Kanat-Alexander]

Attachment: JSON-RPC, v4

And here's an updated patch for JSON-RPC. This one was a bit more complex to update, hopefully I got everything right. :-)

Comment 39

[Frédéric Buclin]

(In reply to comment #28)
>   It wouldn't matter--JSON::RPC::Common also uses Moose. More and more of CPAN
> is using Moose, too--we can't escape it forever.

I think that for now, we can. Are you using features which are specific to Moose and are not available in Mouse? Not everybody is going to use Bugzilla with mod_perl, especially because you cannot have several applications running mod_perl when Bugzilla is in the game.

And the dependencies for Moose are huge compared to Mouse:

 http://deps.cpantesters.org/?module=Moose;perl=latest
 http://deps.cpantesters.org/?module=Mouse;perl=latest


(In reply to comment #28)
>   That is already pretty slow under mod_cgi.

That's not a reason to make things slower. This really makes me think that Bugzilla is becoming only suitable for large installations with large infrastructures. It should still be fast for small installations with limited performances.

Comment 40

[Frédéric Buclin]

Comment on attachment 480380 [details] [diff] [review]
Base, v4

>=== modified file 'Bugzilla/Install/Requirements.pm'

You explicitly |use Moose| in some Bugzilla modules. So you must explicitly list it here.


But as I said in my previous comment, I doubt you need all the features Moose offers and which are not in Mouse.

Comment 41

[Max Kanat-Alexander]

(In reply to comment #39)
> I think that for now, we can. Are you using features which are specific to
> Moose and are not available in Mouse?

  That's irrelevant, because JSON::RPC::Common is using Moose, and it *can't* use Mouse. Also, Mouse is strongly recommended not to be used.

> Not everybody is going to use Bugzilla
> with mod_perl, especially because you cannot have several applications running
> mod_perl when Bugzilla is in the game.

  That's one reason why we want to implement FastCGI for Bugzilla 4.2.

> And the dependencies for Moose are huge compared to Mouse:

  That really should not matter.

Comment 42

[Max Kanat-Alexander]

(In reply to comment #40)
> You explicitly |use Moose| in some Bugzilla modules. So you must explicitly
> list it here.

  I only use Moose because RPC::Any uses Moose, so we don't really need to list it.

Comment 43

[Frédéric Buclin]

(In reply to comment #41)
>   That's irrelevant, because JSON::RPC::Common is using Moose, and it *can't*
> use Mouse. Also, Mouse is strongly recommended not to be used.

It's relevant as you are choosing JSON::RPC::Common. You know it uses Moose.


> > And the dependencies for Moose are huge compared to Mouse:
> 
>   That really should not matter.

It matters e.g. on Windows where it's not trivial to compile/install a Perl module yourself, when the one provided by e.g. PPM is broken.

Comment 44

[Max Kanat-Alexander]

(In reply to comment #43)
> It's relevant as you are choosing JSON::RPC::Common. You know it uses Moose.

  Yes, of course I know it uses Moose, but JSON::RPC::Common IS WHAT RPC::Any USES. Also, it's the only decent JSON::RPC 2.0 implementation on CPAN, and it makes the implementation of RPC::Any way better.

> It matters e.g. on Windows where it's not trivial to compile/install a Perl
> module yourself, when the one provided by e.g. PPM is broken.

  Modern versions of ActiveState can use install-module.pl, we should be recommending ActiveState anymore anyhow, and if a module provided by PPM is broken, then why don't you report a bug to ActiveState and have them fix it? At the very worst, you can go into #win32 on irc.perl.org and talk to the ActiveState guys there about it.

  There are always solutions to everything. Instead of blocking the way forward for Bugzilla, how about we discuss what can be done to make things work? One of the critical things that needs to be done to make XML-RPC work *at all* for most people is for us to stop using SOAP::Lite. Also, this gets us MUCH better JSON-RPC support, in addition to probably getting us SOAP support in the future. Now, I know that you're not intimately familiar with the WebServices code or our WebServices userbase, but I can promise you that everything provided by this module is something that our actual users are going to actually want. The performance aspects can be resolved by making Bugzilla also support FastCGI, which everybody can run, in every web server.

Comment 45

[Max Kanat-Alexander]

Okay, so I've been running WebService QA tests on my local machine under mod_cgi for 4.0, and I'll say that even now, mod_cgi performance is so miserable that nobody should be using mod_cgi if they seriously want to do a lot of WebServices API work. That is, people always should have been using mod_perl if they wanted reasonable WebService performance, so I don't really think that this patch will change that fact at all.

Comment 46

[David Lawrence [:dkl]]

Comment on attachment 480380 [details] [diff] [review]
Base, v4

Bit-rot:

[dkl@localhost trunk]$ patch -p0 < ~/Downloads/rpc-any-base.diff 
patching file Bugzilla.pm
patching file Bugzilla/Constants.pm
patching file Bugzilla/Error.pm
patching file Bugzilla/Install/Requirements.pm
Hunk #1 FAILED at 243.
Hunk #2 succeeded at 265 (offset 1 line).
1 out of 2 hunks FAILED -- saving rejects to file Bugzilla/Install/Requirements.pm.rej
patching file Bugzilla/WebService.pm
patching file Bugzilla/WebService/Constants.pm
Hunk #1 succeeded at 151 with fuzz 2 (offset 20 lines).
Hunk #2 succeeded at 161 (offset 21 lines).
patching file Bugzilla/WebService/Server.pm
Hunk #1 FAILED at 16.
1 out of 1 hunk FAILED -- saving rejects to file Bugzilla/WebService/Server.pm.rej
patching file Bugzilla/WebService/Util.pm
patching file t/002goodperl.t
patching file template/en/default/global/code-error.html.tmpl
Hunk #1 succeeded at 445 (offset 2 lines).

Comment 47

[Max Kanat-Alexander]

Attachment: Base, v4.1

Okay, bitrot fixed.

Comment 48

[David Lawrence [:dkl]]

Bitrot in rpc-any-jsonrpc.diff as well. I will fix it locally so I can continue to review.

[dkl@localhost trunk]$ patch -p0 < ~/Downloads/rpc-any-jsonrpc.diff 
patching file Bugzilla/WebService/Server/JSONRPC.pm
Hunk #3 FAILED at 125.
1 out of 3 hunks FAILED -- saving rejects to file Bugzilla/WebService/Server/JSONRPC.pm.rej
patching file jsonrpc.cgi
patching file template/en/default/global/user-error.html.tmpl
Hunk #1 succeeded at 995 (offset 11 lines).

Comment 49

[Max Kanat-Alexander]

(In reply to comment #48)
> Bitrot in rpc-any-jsonrpc.diff as well. I will fix it locally so I can continue
> to review.

  Really? I just tested it a few days ago. It requires the Base patch. (And only applies on trunk.)

Comment 50

[David Lawrence [:dkl]]

(In reply to comment #49)
> (In reply to comment #48)
> > Bitrot in rpc-any-jsonrpc.diff as well. I will fix it locally so I can continue
> > to review.
> 
>   Really? I just tested it a few days ago. It requires the Base patch. (And
> only applies on trunk.)

Not exactly sure why it is not working for me. Even tried it with a fresh checkout of trunk to be certain.

[dkl@localhost bugzilla-upstream]$ bzr co bzr+ssh://[e-mail]@bzr.mozilla.org/bugzilla/trunk trunk-new
[dkl@localhost trunk]$ cd trunk-new
[dkl@localhost trunk-new]$ patch -p0 < ~/Downloads/rpc-any-base.diff
patching file Bugzilla.pm
patching file Bugzilla/Constants.pm
patching file Bugzilla/Error.pm
patching file Bugzilla/Install/Requirements.pm
...
[dkl@localhost trunk-new]$ patch -p0 < ~/Downloads/rpc-any-xmlrpc.diff 
patching file Bugzilla/WebService/Server/XMLRPC.pm
patching file template/en/default/global/user-error.html.tmpl
Hunk #1 succeeded at 1693 (offset 46 lines).
patching file xmlrpc.cgi
[dkl@localhost trunk-new]$ patch -p0 < ~/Downloads/rpc-any-jsonrpc.diff 
patching file Bugzilla/WebService/Server/JSONRPC.pm
Hunk #3 FAILED at 125.
1 out of 3 hunks FAILED -- saving rejects to file Bugzilla/WebService/Server/JSONRPC.pm.rej
patching file jsonrpc.cgi
patching file template/en/default/global/user-error.html.tmpl
Hunk #1 succeeded at 995 (offset 11 lines).

Comment 51

[Max Kanat-Alexander]

Attachment: JSON-RPC, v4.1

Okay, this fixes the bitrot.

Comment 52

[David Lawrence [:dkl]]

Comment on attachment 480381 [details] [diff] [review]
XML-RPC, v4

Okay I have gone through this as much as I think I can at the moment. The code changes in the Bugzilla core modules looks good to me. I have not delved into the code of RPC::Any much as I assume you don't really need a review for that so much. I have learned some about Moose in the process and understand mostly what you are doing where you have used it in the Bugzilla code. All of my sanity tests including the webservices test suite and my own test scripts have passed as well. Performance questions aside, I feel like this is good to go. r=dkl

Comment 53

[David Lawrence [:dkl]]

Comment on attachment 505979 [details] [diff] [review]
Base, v4.1

Similar comments from XMLRPC patch review. r=dkl

Comment 54

[David Lawrence [:dkl]]

Comment on attachment 507689 [details] [diff] [review]
JSON-RPC, v4.1

Similar comments from the XMLRPC patch review. r=dkl

Comment 55

[Frédéric Buclin]

Note that I definitely object to check in these patches for performance reasons, as already said several times. mkanat doesn't have the power to bypass my a-. I ask justdave to comment in this bug as project leader.

Comment 56

[Max Kanat-Alexander]

(In reply to comment #55)
> Note that I definitely object to check in these patches for performance
> reasons, as already said several times. mkanat doesn't have the power to bypass
> my a-. I ask justdave to comment in this bug as project leader.

  Hey. I understand your objection, but a- is really inappropriate, but you could certainly r- a patch, or say, "we should support fastcgi first".

Comment 57

[Max Kanat-Alexander]

  Also, neither you nor I have any hard numbers nor any idea about actual performance differences. I will get you those actual numbers.

Comment 58

[Dave Miller [:justdave]]

What kind of performance impact are we talking here?  Only affects RPC or will slow down other stuff by virtue of being loaded?  I've had more than one person tell me that the RPC is usable under mod_cgi currently, does it get significantly worse with the new setup?

On the one hand, getting rid of SOAP::Lite excites me.  But it sorta sounds like we're trading one disaster for another here.

Comment 59

[Dave Miller [:justdave]]

oops, clicked the wrong button on my mid-air override there, didn't mean to remove the dependency.  Sounds like Max (comment 57) is getting the info I was interested in.  Definitely want to see that.

Comment 60

[Frédéric Buclin]

(In reply to comment #56)
>   Hey. I understand your objection, but a- is really inappropriate, but you
> could certainly r- a patch, or say, "we should support fastcgi first".

I already r- a patch; that was attachment 480380 [details] [diff] [review]. But then you simply attach another patch and the r- is not applicable anymore. The a- is because we already discussed this problem, and we should as a minimum implement support for FastCGI first, and see how things work (and make sure it also works fine with IIS). That's why reed set the dependency on bug 316665.

Comment 61

[Max Kanat-Alexander]

Okay, I'm testing webservice_bug_get_bugs.t.

current code under mod_perl:
10 wallclock secs ( 0.04 usr  0.00 sys +  0.32 cusr  0.03 csys =  0.39 CPU)

mod_perl + RPC::Any:
10 wallclock secs ( 0.03 usr  0.00 sys +  0.32 cusr  0.03 csys =  0.38 CPU)

current code under mod_cgi:
30 wallclock secs ( 0.03 usr  0.00 sys +  0.34 cusr  0.03 csys =  0.40 CPU)

mod_cgi + RPC::Any:
40 wallclock secs ( 0.04 usr  0.01 sys +  0.32 cusr  0.04 csys =  0.41 CPU)

Just for kicks, here's how it looks if I only do XML-RPC tests or only JSON-RPC tests for each one:

RPC::Any XML-RPC:
15 wallclock secs ( 0.03 usr  0.00 sys +  0.26 cusr  0.02 csys =  0.31 CPU)

RPC::Any JSON-RPC:
16 wallclock secs ( 0.02 usr  0.00 sys +  0.19 cusr  0.02 csys =  0.23 CPU)

current XML-RPC:
11 wallclock secs ( 0.03 usr  0.01 sys +  0.26 cusr  0.03 csys =  0.33 CPU)

current JSON-RPC:
11 wallclock secs ( 0.03 usr  0.00 sys +  0.21 cusr  0.01 csys =  0.25 CPU)

So yes, the RPC::Any code is slower under mod_cgi than the current code is, but what these tests primarily show us is that if you're running under mod_cgi, the WebService is slow. Nobody should ever expect the WebService to perform reasonably under mod_cgi--it doesn't now. Anybody who needs performance from the WebService should be using mod_perl, because if they needed performance now, they'd already be using mod_perl. The perceived performance of the WebService under mod_cgi would likely be totally the same for all consumers.

On the other hand, the advantages that RPC::Any could bring us are pretty awesome:

* First off, it results in way cleaner code, which makes it way easier for us to maintain and improve the WebService.

* Secondly, it gets us JSON-RPC 2.0 support, which is the only version of JSON-RPC that people should actually be *using*. All other versions of JSON-RPC are basically the past. JSON-RPC 2.0 is good, JSON-RPC 1.x is bad. However, we retain *full* support for 1.0 and 1.1 (actually even better support than we have now) thanks to the magic of RPC::Any.

* It gets us way better support for <nil/> in XML-RPC, which allows passing undef around as appropriate. (Our current libraries make this very difficult.)

* SOAP::Lite is broken pretty badly, to the point that current versions don't work at all under mod_perl. I've communicated with the maintainers back and forth for months, and they seem uninterested in actually doing the work required (even though it's just a few lines of code) to fix it. On top of that, SOAP::Lite seems to rarely have bug-fix releases. Compare that to RPC::Any, which has never even had a bug reported against it, thanks to the full-coverage test suite, and which was explicitly designed to work under both mod_cgi and mod_perl.

* SOAP::Lite also has a history of broken Unicode support. RPC::Any, on the other hand, has a test suite of hundreds of tests that prove that Unicode support works in every possible situation.

* RPC::Any natively works the way that we need a WebService module to work, instead of us having to hack a tremendous amount of garbage on top of other CPAN libraries to get our WebServices to work.

* RPC::Any has a *way* better system for catching and throwing errors than any of our current WebService libraries do--this means that clients will *always* get a *WebService* error when there is a problem, and never get a Perl error instead (which causes their WebService client to die).

* RPC::Any has native taint support and protection, so we don't have to hack anything about that in.

* RPC::Any natively supports the standard -32xxx error codes for JSON-RPC and XML-RPC, so we don't have to maintain the standard codes.

* Finally, one of the biggest advantages is that any improvements in RPC::Any will also become (or quickly lead to) improvements in Bugzilla. Want SOAP support for the Bugzilla WebService? Just implement a SOAP backend for RPC::Any. Want to support REST for our WebServices? That would just be some new modules in RPC::Any, which already has a whole framework for doing these sorts of things.

Comment 62

[Frédéric Buclin]

So what we see is a 30-40% penalty on mod_cgi with RPC::Any. Not all installations have a huge traffic which requires mod_perl, but a 30-40% penalty would definitely be noticeable to them.

Comment 63

[Max Kanat-Alexander]

(In reply to comment #62)
> So what we see is a 30-40% penalty on mod_cgi with RPC::Any. Not all
> installations have a huge traffic which requires mod_perl, but a 30-40% penalty
> would definitely be noticeable to them.

  Okay. If you have personally interacted with WebService consumers or done some research on how people use the WebService, could you give some concrete examples?

Comment 64

[Max Kanat-Alexander]

Oh, by the way, another advantage of RPC::Any is that it should be pretty easy to add system.multicall support for XML-RPC and batched requests support for JSON-RPC, meaning that multiple calls can be done in one request.

Comment 65

[Frédéric Buclin]

(In reply to comment #63)
> (In reply to comment #62)
>   Okay. If you have personally interacted with WebService consumers or done
> some research on how people use the WebService, could you give some concrete
> examples?

Bugzilla 4.0 itself uses WebService for the auto-complete stuff, and Bugzilla 4.2 should use WebService for tags soon. If the time taken by WS is increased by 30-40% here, then yes, that's noticeable.

Comment 66

[Max Kanat-Alexander]

(In reply to comment #65)
> Bugzilla 4.0 itself uses WebService for the auto-complete stuff, and Bugzilla
> 4.2 should use WebService for tags soon. If the time taken by WS is increased
> by 30-40% here, then yes, that's noticeable.

  Well, I just did some timing of autocomplete, and it's currently about 1.6 seconds to get a result on mod_cgi with the current code, versus about 2.1 seconds to get a result with rpc-any. I'm not sure that people will notice anything other than that both of them suck.

Comment 67

[Dave Miller [:justdave]]

Out of curiosity, you suppose we could track down anyone that uses this feature on a mod_cgi Bugzilla and get their impressions of the current version, and whether they think a couple extra seconds delay would kill anything?

That's one of the complaints I'm (still) hearing frequently about Bugzilla is performance (even though it's *LOADS* better than it used to be - I guess people get more and more impatient as the internet matures or something ;) so anything that makes performance worse worries me (even for the great benefits we get from a developer perspective.

The number of dependencies RPC::Any has worries me, too.  We're going to have to start shipping lib/ pre-stocked with all the Perl modules soon or it's going to be a nightmare to install (and that's going to be tricky with some of the dependencies having binary components) and people already complain about installing dependencies for Bugzilla.

Comment 68

[Max Kanat-Alexander]

(In reply to comment #67)
> Out of curiosity, you suppose we could track down anyone that uses this feature
> on a mod_cgi Bugzilla and get their impressions of the current version, and
> whether they think a couple extra seconds delay would kill anything?

  Well, you could try it yourself:

  http://landfill.bugzilla.org/bugzilla-4.0-branch/show_bug.cgi?id=1

  That's mod_cgi.

> That's one of the complaints I'm (still) hearing frequently about Bugzilla is
> performance (even though it's *LOADS* better than it used to be - I guess
> people get more and more impatient as the internet matures or something ;) so
> anything that makes performance worse worries me (even for the great benefits
> we get from a developer perspective.

  Ah, yeah, but all the complaints I've heard are about search performance, not about anything else. Having used several other bug-trackers, I'm pretty sure we're still the fastest (maybe other than Google Code's).

> The number of dependencies RPC::Any has worries me, too.  We're going to have
> to start shipping lib/ pre-stocked with all the Perl modules soon or it's going
> to be a nightmare to install (and that's going to be tricky with some of the
> dependencies having binary components) and people already complain about
> installing dependencies for Bugzilla.

  Well, just think of all those dependencies as code we don't have to write. :-) Also, since the advent of install-module.pl (and particularly since I improved it in 3.6.3), I rarely hear anybody complain now about installing dependencies.

Comment 69

[Frédéric Buclin]

(In reply to comment #68)
> :-) Also, since the advent of install-module.pl (and particularly since I
> improved it in 3.6.3), I rarely hear anybody complain now about installing
> dependencies.

This is not error prone, see bug 621591 comment 45. Vance was unable to install Math::Random::Secure on SUSE 10 SP2 because one of the dependencies refused to install correctly.

Comment 70

[Dave Miller [:justdave]]

We also didn't manage to get it working on bmo and had to remove it to keep the site working.

Comment 71

[David Lawrence [:dkl]]

(In reply to comment #67)
> The number of dependencies RPC::Any has worries me, too.  We're going to have
> to start shipping lib/ pre-stocked with all the Perl modules soon or it's going
> to be a nightmare to install (and that's going to be tricky with some of the
> dependencies having binary components) and people already complain about
> installing dependencies for Bugzilla.

Not to mention all of the installations that have to have all of the dependencies as rpms on supported enterprise linux distros. These distros most times either do not have all of CPAN already provided or the ones that are are very out of date. The developer then has to manually build these dependencies themselves if not available via some other source such as rpmforge. I speak of this from experience :) Also at Red Hat, the policy was that everything had to be built and signed through the in house build system so even if it was already packaged somewhere else we couldn't use it. So point being, the less extra dependencies the better.

As for performance at Red Hat, the webservices API was notoriously slow and some people from time to time did complain. Eventually people just became numb to it and just accepted it. We could not move to mod_perl because of the server hosting other sites at the time besides Bugzilla. There were plans to create a separate server just for webservices requests to divide up the load but it was never deployed due to other critical projects. Hopefully Red Hat will eventually be running on mod_perl before 4.2 is released but if not I am sure that they will not be happy if the performance is even slightly worse than it is now.

Dave

Comment 72

[Max Kanat-Alexander]

(In reply to comment #70)
> We also didn't manage to get it working on bmo and had to remove it to keep the
> site working.

  Yes, but that's a bug that should be fixed--one we don't even know the cause of yet. So we shouldn't be using that as a basis for speculation or further decisions until we understand it. I can tell you that on my RHEL5 and Fedora systems, Math::Random::Secure was installed and worked perfectly with install-module, and the same is true for RPC::Any.

(In reply to comment #71)
> Not to mention all of the installations that have to have all of the
> dependencies as rpms on supported enterprise linux distros.

  The easy solution to that, for most people, is to add RPC::Any to rpmforge. Even if you then have to build the RPM yourself, you can at least import all the spec files or SRPMs from rpmforge.

> So point being, the less extra dependencies the better.

  Fair enough, but that's clearly not the important criterion here from a software design perspective. I mean, the alternative is "write everything ourselves", which would be obvious idiocy. Or depend on libraries where people wrote everything themselves even if there were already other libraries that implemented everything they were doing, which would just be transferring that exact same idiocy from us to them.

  The nature of Perl and the CPAN is that it has long dependency chains. If you don't want that, we should re-write Bugzilla in another language.

> As for performance at Red Hat, the webservices API was notoriously slow and
> some people from time to time did complain. [snip] I am sure
> that they will not be happy if the performance is even slightly worse than it
> is now.

  If there's an invading army that kills 1000 people, the question we should be asking is not, "How do we prevent the death of 300 more people," but "How do get the army to stop attacking so that no more people die at all?"

  In our case, I suspect the answer is FastCGI.

  So if the decision is "We should support FastCGI before checking this patch in," then that's all that needs to be said. We don't need to go back and forth about this.

  Anyhow, setting this back to "a?" to hold it in our queue, just like we've done with every other bug that's in this same situation for the last six years.

Comment 73

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 74

[Dave Miller [:justdave]]

Based on comments on CPAN it appears RPC::Any no longer has a maintainer.  While I applaud the concept, the current implementation carries a performance penalty that I don't want to take, and without a maintainer to address those performance penalties there's not much reason to continue looking here.

Comment 75

[Dave Miller [:justdave]]

The original reasoning for wanting it was to get away from problems with SOAP::Lite, which we probably still need to address.  Given the amount of confusion on this bug we're probably best off starting over with a new bug for that in general.