rpc.js updates, optimizations, and info

[John Hjelmstad <fa...@google.com>]

Hello all,
Many of you have likely seen a series of rpc.js-related changes I've made in
the past few weeks. These implemented several rpc.js improvements:

1. RMR, new transport for Safari < 4, Chrome < 2, and should work as a
backup on several other browsers (the latter claim requires more testing).
  - Paves the way to sunsetting IFPC, in turn sunsetting the need for
containers to host "active" relay file rpc_relay.html and configure it
properly.

2. Refactoring transport code into separate "classes". This cleans up the
code a bit and makes it possible to optimize later by emitting only the JS a
given browser needs rather than JS for all techniques.

3. Early-message queueing. Makes it possible to call gadgets.rpc.call(...)
in a container to a gadget before the gadget is initialized (or even
rendered), with the call still going through when the gadget is ready. It
also ensures that G -> C calls made before G -> C initialization is complete
also succeed (this is much less prevalent).

4. Various cleanups including tentative backward-setup support. As a
convenience and to prevent user error, makes it possible to call
gadgets.rpc.setAuthToken(gadget, token); before the gadget IFRAME is
rendered. While not recommended in any case, this is intended to make the
library easier to use.

At Google we found several pain points and subtle bugs associated with the
combination of these features, so I've rolled back the code for the moment
to give others another look at it. The code is here:
http://codereview.appspot.com/63209

A few things learned during this process:

* During integration testing of this library, it became exceedingly clear
that having automated integration testing for as many configurations of it
as possible is a must. In particular, one should test each browser,
container config, gadget server build, and container build (ideally the
matrix of all). We have implemented this within Google and will be
subjecting all rpc improvements to it.

* Containers taking an rpc.js "snapshot" make improvements/upgrades
exceedingly difficult to impossible. As has been noted in several places,
the rpc.js version that the container loads should always be the exact same
as what the gadget loads. This is often achieved by the container including
<script src="
http://gadgetserver.com/gadgets/js/rpc.js?c=1&container=1[&debug=1][&v=
<hash>]"></script>.

In addition, I've implemented the base support for per-browser rpc.js
optimization on the server side, mentioned as the rationale for rpc.js
refactoring (#2 above). This code is experimental for the moment, and any
real-world deployment should be handled very carefully. Code reviewer link:
http://codereview.appspot.com/63210

Best,
John

Versioning of JavaScript

["Weygandt, Jon" <jw...@ebay.com>]

This topic comes up from a comment on the rpc.js updates, see below.

The simplistic method below works only if you have one physical server,
or can simultaneously update your server farm. But if you have a gradual
roll out to the servers, a version skew can start to take place. This is
a problem we will be facing in the next few months.

My first thought is a custom GadgetFeatureRegistry and the use Guice,
but if there is a need in the community for a solution, perhaps we can
start to scheme on how that could be done as it might require some
substantial changes.

A high level proposal, just to get people thinking: 

The GadgetFeatureRegistry would support versioning, and the feature JS
would have to be versioned as well (of course only features with gadget
and container code, like rpc.js). A configuration variable would tell
the server what version to serve up. The metadata call would return that
version to the container so it can generate the proper js servlet call
and ifr render call.

A new build would be made with a minimum of old and new JS versions. But
the configuration would point to the old version. A roll out to all the
servers would commence, and over time finish. The system would run new
server side executable code, but the old JS code. The configuration
variable would be altered, at runtime of course, to switch the JS
versions to the new version. 

Even if the switching takes time, or pages are cached, all servers can
serve both old and new versions, and the container will coordinate the
js and ifr servlet calls correctly.

Jon

-----Original Message-----
From: John Hjelmstad [mailto:fargo@google.com] 
Sent: Monday, June 08, 2009 7:21 PM
To: shindig-dev@incubator.apache.org
Subject: rpc.js updates, optimizations, and info

[deleted...]
* Containers taking an rpc.js "snapshot" make improvements/upgrades
exceedingly difficult to impossible. As has been noted in several
places, the rpc.js version that the container loads should always be the
exact same as what the gadget loads. This is often achieved by the
container including <script src="
http://gadgetserver.com/gadgets/js/rpc.js?c=1&container=1[&debug=1][&v=
<hash>]"></script>.

[deleted...]
Best,
John

Re: rpc.js updates, optimizations, and info

[John Hjelmstad <fa...@google.com>]

Any time something breaks, it's easy to look back and admonish oneself for
what didn't work out. I know I have.
My challenge is pragmatically answering for this lib, what constitutes
proper peer review of this code? What's "large"? Many bugs came from small
changes. The "largest" changes were RMR (peer reviewed, still had bugs found
in testing) and a code move/refactor (announced on this list, tested
extensively locally, still had issues in real-world deployments).
To date, only Brian Eaton and Joey Schorr have shown much interest
(reticence is understandable, if you ask me) to look at this code in depth.
The fact is, it's a gnarly bunch of browser hacks deployed and used in all
sorts of ways, often unanticipated but ways that end up needing to be
supported. So testing has been just about the only way that bugs have been
found in it. To that end, I've improved the test harness in Shindig, spent
countless hours (sanity...dwindling...) using it, and as mentioned, we'll
have automated per-browser testing within Google, which is an effective
requirement since as many bugs have been found in "real" deployments as in a
"standard" container/gadget integration.

I'm as big a fan of extensive peer review as anybody. If nothing else, it
gives me some cover especially on this nasty lib :) I also know there are
problems with the library as it stands, and want to find a proper balance in
improving them. The good news is this patch is pretty well tested at this
point. All constructive feedback welcome.

--j

On Tue, Jun 9, 2009 at 10:54 AM, Kevin Brown <et...@google.com> wrote:

> On Mon, Jun 8, 2009 at 7:20 PM, John Hjelmstad <fa...@google.com> wrote:
>
> > Hello all,
> > Many of you have likely seen a series of rpc.js-related changes I've made
> > in
> > the past few weeks. These implemented several rpc.js improvements:
> >
> > 1. RMR, new transport for Safari < 4, Chrome < 2, and should work as a
> > backup on several other browsers (the latter claim requires more
> testing).
> >  - Paves the way to sunsetting IFPC, in turn sunsetting the need for
> > containers to host "active" relay file rpc_relay.html and configure it
> > properly.
> >
> > 2. Refactoring transport code into separate "classes". This cleans up the
> > code a bit and makes it possible to optimize later by emitting only the
> JS
> > a
> > given browser needs rather than JS for all techniques.
> >
> > 3. Early-message queueing. Makes it possible to call
> gadgets.rpc.call(...)
> > in a container to a gadget before the gadget is initialized (or even
> > rendered), with the call still going through when the gadget is ready. It
> > also ensures that G -> C calls made before G -> C initialization is
> > complete
> > also succeed (this is much less prevalent).
> >
> > 4. Various cleanups including tentative backward-setup support. As a
> > convenience and to prevent user error, makes it possible to call
> > gadgets.rpc.setAuthToken(gadget, token); before the gadget IFRAME is
> > rendered. While not recommended in any case, this is intended to make the
> > library easier to use.
> >
> > At Google we found several pain points and subtle bugs associated with
> the
> > combination of these features, so I've rolled back the code for the
> moment
> > to give others another look at it. The code is here:
> > http://codereview.appspot.com/63209
> >
>
> >
> > A few things learned during this process:
> >
> > * During integration testing of this library, it became exceedingly clear
> > that having automated integration testing for as many configurations of
> it
> > as possible is a must. In particular, one should test each browser,
> > container config, gadget server build, and container build (ideally the
> > matrix of all). We have implemented this within Google and will be
> > subjecting all rpc improvements to it.
> >
> > * Containers taking an rpc.js "snapshot" make improvements/upgrades
> > exceedingly difficult to impossible. As has been noted in several places,
> > the rpc.js version that the container loads should always be the exact
> same
> > as what the gadget loads. This is often achieved by the container
> including
> > <script src="
> > http://gadgetserver.com/gadgets/js/rpc.js?c=1&container=1[&debug=1][&v=<
> http://gadgetserver.com/gadgets/js/rpc.js?c=1&container=1%5B&debug=1%5D%5B&v=
> >
> > <hash>]"></script>.
>
>
> You forgot the most important lesson: Nobody is supposed to be committing
> large patches without review. That is completely unacceptable and has
> always
> been. This code wasn't hosed because of the extent of the changes or the
> nature of rpc.js itself, it was hosed because it was not properly peer
> reviewed. Even the most alert and bright person will make mistakes in any
> patch of this size, and that's why we review big patches before we commit
> them.
>
>
> >
> >
> > In addition, I've implemented the base support for per-browser rpc.js
> > optimization on the server side, mentioned as the rationale for rpc.js
> > refactoring (#2 above). This code is experimental for the moment, and any
> > real-world deployment should be handled very carefully. Code reviewer
> link:
> > http://codereview.appspot.com/63210
> >
> > Best,
> > John
> >
>

Re: rpc.js updates, optimizations, and info

[Kevin Brown <et...@google.com>]

On Mon, Jun 8, 2009 at 7:20 PM, John Hjelmstad <fa...@google.com> wrote:

> Hello all,
> Many of you have likely seen a series of rpc.js-related changes I've made
> in
> the past few weeks. These implemented several rpc.js improvements:
>
> 1. RMR, new transport for Safari < 4, Chrome < 2, and should work as a
> backup on several other browsers (the latter claim requires more testing).
>  - Paves the way to sunsetting IFPC, in turn sunsetting the need for
> containers to host "active" relay file rpc_relay.html and configure it
> properly.
>
> 2. Refactoring transport code into separate "classes". This cleans up the
> code a bit and makes it possible to optimize later by emitting only the JS
> a
> given browser needs rather than JS for all techniques.
>
> 3. Early-message queueing. Makes it possible to call gadgets.rpc.call(...)
> in a container to a gadget before the gadget is initialized (or even
> rendered), with the call still going through when the gadget is ready. It
> also ensures that G -> C calls made before G -> C initialization is
> complete
> also succeed (this is much less prevalent).
>
> 4. Various cleanups including tentative backward-setup support. As a
> convenience and to prevent user error, makes it possible to call
> gadgets.rpc.setAuthToken(gadget, token); before the gadget IFRAME is
> rendered. While not recommended in any case, this is intended to make the
> library easier to use.
>
> At Google we found several pain points and subtle bugs associated with the
> combination of these features, so I've rolled back the code for the moment
> to give others another look at it. The code is here:
> http://codereview.appspot.com/63209
>

>
> A few things learned during this process:
>
> * During integration testing of this library, it became exceedingly clear
> that having automated integration testing for as many configurations of it
> as possible is a must. In particular, one should test each browser,
> container config, gadget server build, and container build (ideally the
> matrix of all). We have implemented this within Google and will be
> subjecting all rpc improvements to it.
>
> * Containers taking an rpc.js "snapshot" make improvements/upgrades
> exceedingly difficult to impossible. As has been noted in several places,
> the rpc.js version that the container loads should always be the exact same
> as what the gadget loads. This is often achieved by the container including
> <script src="
> http://gadgetserver.com/gadgets/js/rpc.js?c=1&container=1[&debug=1][&v=<http://gadgetserver.com/gadgets/js/rpc.js?c=1&container=1%5B&debug=1%5D%5B&v=>
> <hash>]"></script>.


You forgot the most important lesson: Nobody is supposed to be committing
large patches without review. That is completely unacceptable and has always
been. This code wasn't hosed because of the extent of the changes or the
nature of rpc.js itself, it was hosed because it was not properly peer
reviewed. Even the most alert and bright person will make mistakes in any
patch of this size, and that's why we review big patches before we commit
them.


>
>
> In addition, I've implemented the base support for per-browser rpc.js
> optimization on the server side, mentioned as the rationale for rpc.js
> refactoring (#2 above). This code is experimental for the moment, and any
> real-world deployment should be handled very carefully. Code reviewer link:
> http://codereview.appspot.com/63210
>
> Best,
> John
>