AJAX request deduplication. #1309

Merged
merged 9 commits into from Aug 27, 2012

Conversation

Projects
None yet
3 participants
@Shadowfiend
Member

Shadowfiend commented Aug 18, 2012

Regular AJAX requests now carry a single-character version identifier.
Server-side, if we've already seen a request for that identifier (meaning
this is a retry), we make the request wait for the original processing code
to finish before returning the response from there. If the original request
has already completed, we return the existing response for that request.

Continuations are used to wait for the completion of the initial request when
needed and available.

Shadowfiend added some commits Aug 18, 2012

Add !< to LiftCometActor.
CometActor had the function by virtue of extending Lift's actor traits;
however, the LiftCometActor trait that is often used to reference it
didn't define that the method would be present. We add it there so it
can be referred to anywhere that LiftCometActor is passed around instead
of CometActor.

!< returns an LAFuture for a message reply.
Include a version on the end of the Ajax GUID.
AJAX requests currently carry a GUID. We now add a single-character
version indicator. This version increments for distinct AJAX requests.
In particular, it does NOT increment during AJAX retries, so that a
retry of an existing request can be identified as being part of the same
attempt on the server.

Not all AJAX requests carry this version identifier. In particular, Lift
GC requests do not carry a version identifier. This is because these
are going to be handled in a streamlined handler. If a Lift GC request
doesn't make it through, we can retry it as many times as we want and
it'll just remark stuff in the session.

liftAjax.addPageNameAndVersion appends both the page GUID and the
version number. The version number is encoded in base-36.
Rename continuation-related code to clarify Comet association.
We're about to add some AJAX-related continuation code, so we want it to
be clear the existing stuff is for comets.
AJAX version-based deduplication.
The meat of the deal. Based on the AJAX version appended to the request
GUID, we determine whether we've already seen this request. If so, we
wait for the original request to complete before returning the resulting
value.  If we already completed the request before, we return the same
answer without re-running the associated parameters. AJAX requests that
need to wait are put into continuations if available.
Delimit version in GUID string by a dash.
Before we were relying on the expected length of the funcName,
determined by calling nextFuncName. Because funcName isn't *always* the
same length, we switch instead ot putting a - between the GUID and the
version identifier in the path. We then look for it when extracting the
version identifier.
Move ajax request list into LiftSession with lastSeen.
LiftSession is in charge of managing cleanup of non-recently-seen pages
and such, so it needs to know about the AjaxRequestInfo list to clean it
up when a given request hasn't been needed in sufficiently long.
@Shadowfiend

This comment has been minimized.

Show comment
Hide comment
@Shadowfiend

Shadowfiend Aug 18, 2012

Member

This is some pretty hairy stuff and required reworking a decent bit of the AJAX handling, so please test it and go over it a bit. I'm going to run some more tests myself (I'm testing the two basic AJAX processing scenarios (within and without a comet) each with two different delay potentials (no delay and 9s of delay) at https://github.com/Shadowfiend/lift-tests , amongst other things.

Once I get some feedback from you folks, I'll probably also put it out on OpenStudy and make sure things run smoothly there.

Member

Shadowfiend commented Aug 18, 2012

This is some pretty hairy stuff and required reworking a decent bit of the AJAX handling, so please test it and go over it a bit. I'm going to run some more tests myself (I'm testing the two basic AJAX processing scenarios (within and without a comet) each with two different delay potentials (no delay and 9s of delay) at https://github.com/Shadowfiend/lift-tests , amongst other things.

Once I get some feedback from you folks, I'll probably also put it out on OpenStudy and make sure things run smoothly there.

Don't suspend requests too early for the first request.
We were suspending the request before we got a chance to kick off the
processing for the actual response to be run.
@Shadowfiend

This comment has been minimized.

Show comment
Hide comment
@Shadowfiend

Shadowfiend Aug 18, 2012

Member

Hm. Also I guess I screwed up the pull request attaching to an existing ticket. This would resolve issue #956.

Member

Shadowfiend commented Aug 18, 2012

Hm. Also I guess I screwed up the pull request attaching to an existing ticket. This would resolve issue #956.

@fmpwizard

This comment has been minimized.

Show comment
Hide comment
@fmpwizard

fmpwizard Aug 20, 2012

Member

Why do you use Box[Box[LiftResponse]] instead of just Box[LiftResponse] ?

Could we run into a situation where we would get a Full(Failure(x,y,z)) ? and then think that the response is still fine?

Why do you use Box[Box[LiftResponse]] instead of just Box[LiftResponse] ?

Could we run into a situation where we would get a Full(Failure(x,y,z)) ? and then think that the response is still fine?

This comment has been minimized.

Show comment
Hide comment
@Shadowfiend

Shadowfiend Aug 20, 2012

Member

So, the ajax runner returns a Box[LiftResponse] as it did before these changes. An Empty or Failure at the top level of the Box[Box[]] means we haven't calculated a response and therefore it should be calculated. A Full() means the runAjax function calculated a response, and that is what the is. The response won't be fine, it will just already be precomputed as a Failure. Does that make sense?

Member

Shadowfiend replied Aug 20, 2012

So, the ajax runner returns a Box[LiftResponse] as it did before these changes. An Empty or Failure at the top level of the Box[Box[]] means we haven't calculated a response and therefore it should be calculated. A Full() means the runAjax function calculated a response, and that is what the is. The response won't be fine, it will just already be precomputed as a Failure. Does that make sense?

This comment has been minimized.

Show comment
Hide comment
@fmpwizard

fmpwizard Aug 20, 2012

Member

It does, thanks for the clarification, would you mind adding it as a comment?

Member

fmpwizard replied Aug 20, 2012

It does, thanks for the clarification, would you mind adding it as a comment?

This comment has been minimized.

Show comment
Hide comment
@Shadowfiend

Shadowfiend Aug 20, 2012

Member

Sure. Yeah I inlined the relevant comment in the returns under the collect. But I think I'll centralize it up top.

Member

Shadowfiend replied Aug 20, 2012

Sure. Yeah I inlined the relevant comment in the returns under the collect. But I think I'll centralize it up top.

@fmpwizard

This comment has been minimized.

Show comment
Hide comment
@fmpwizard

fmpwizard Aug 20, 2012

Member

I was going to ask about how safe it was to use val funcLength = Helpers.nextFuncName.length , glad you went the path of being explicit about the two values.

Member

fmpwizard commented on 1c46530 Aug 20, 2012

I was going to ask about how safe it was to use val funcLength = Helpers.nextFuncName.length , glad you went the path of being explicit about the two values.

This comment has been minimized.

Show comment
Hide comment
@Shadowfiend

Shadowfiend Aug 20, 2012

Member

Yeah, it was a bit of a hack at first to get things going while I figured out a better way of dealing with it :)

Member

Shadowfiend replied Aug 20, 2012

Yeah, it was a bit of a hack at first to get things going while I figured out a better way of dealing with it :)

@fmpwizard

This comment has been minimized.

Show comment
Hide comment
@fmpwizard

fmpwizard Aug 20, 2012

Member

If we use a mutable.Map , couldn't this be a val? It compiles locally if I change it to a val.

If we use a mutable.Map , couldn't this be a val? It compiles locally if I change it to a val.

This comment has been minimized.

Show comment
Hide comment
@Shadowfiend

Shadowfiend Aug 20, 2012

Member

That's an excellent point actually. This was left over from when it was an immutable, but I wanted to use the map as its own synchronization point.

Member

Shadowfiend replied Aug 20, 2012

That's an excellent point actually. This was left over from when it was an immutable, but I wanted to use the map as its own synchronization point.

@fmpwizard

This comment has been minimized.

Show comment
Hide comment
@fmpwizard

fmpwizard Aug 20, 2012

Member

It looks very good, I didn't see anything clearly wrong with it, I just did a publish-local and early this week I'll try it with our app at work as well as another application I have been working on for some time that makes use of some ajax and comet.

Member

fmpwizard commented Aug 20, 2012

It looks very good, I didn't see anything clearly wrong with it, I just did a publish-local and early this week I'll try it with our app at work as well as another application I have been working on for some time that makes use of some ajax and comet.

@Shadowfiend

This comment has been minimized.

Show comment
Hide comment
@Shadowfiend

Shadowfiend Aug 20, 2012

Member

Made the changes you pointed out!

Member

Shadowfiend commented Aug 20, 2012

Made the changes you pointed out!

@fmpwizard

This comment has been minimized.

Show comment
Hide comment
@fmpwizard

fmpwizard Aug 21, 2012

Member

Thanks! I'll post on the mailing list once I get to try this branch on my two applications.

Member

fmpwizard commented Aug 21, 2012

Thanks! I'll post on the mailing list once I get to try this branch on my two applications.

dpp pushed a commit that referenced this pull request Aug 27, 2012

David Pollak
Merge pull request #1309 from lift/asc_issue_956
AJAX request deduplication.

@dpp dpp merged commit aae6082 into master Aug 27, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment