Skip to content

Reprocess failed ajax request.#1366

Closed
jeppenejsum wants to merge 2 commits into
lift:masterfrom
jeppenejsum:retry_ajax_request_on_servererror
Closed

Reprocess failed ajax request.#1366
jeppenejsum wants to merge 2 commits into
lift:masterfrom
jeppenejsum:retry_ajax_request_on_servererror

Conversation

@jeppenejsum

Copy link
Copy Markdown
Member

When an ajax request fails the client will resend the request.

With this patch, if the failure is caused by the server (http code >500) the retry-request will actually be processed again instead of being satisfied by the previously computed response (which was a failure)

When an ajax request fails the client will resend the request.

With this patch, if the failure is caused by the server (http code >500) the retry-request will actually be processed again instead of being satisfied by the previously computed response (which was a failure)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is only used here, I'm not sure having an extra indirection through the CachedResponse class is needed. Why not just perform the code check directly here and act accordingly? Something like:

result match {
  case Full(response) if response.code >= 500 && response.code < 600 =>
    // The request failed...
    liftSession.with...
  case other =>
    future.satisfy(other)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, in this case, for any pending requests, we should also still satisfy the future with the result.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But LiftResponse doesnt have code, only toResponse. So we end up calling toResponse twice (here and where the response is actually sent)

I'm unsure why this is so but assumes there's a reason :-)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I see. That does indeed complicate things. @dpp does this look reasonable to you then?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please give it a try and see how it works in the wild. ;-)

@ghost ghost assigned jeppenejsum Dec 4, 2012
@fmpwizard

Copy link
Copy Markdown
Member

As discussed on the mailing list, closing this pull request until Jeppe (or anyone else) has time to implement "Oh, in this case, for any pending requests, we should also still satisfy the future with the result." (Just to avoid merging it to master until it is ready)

@fmpwizard fmpwizard closed this Dec 4, 2012
@jeppenejsum

Copy link
Copy Markdown
Member Author

Updated. Sorry for the delay :-)

@jeppenejsum jeppenejsum reopened this Dec 4, 2012
@Shadowfiend

Copy link
Copy Markdown
Member

+1 looks good.

@fmpwizard

Copy link
Copy Markdown
Member

rebased to master

@fmpwizard fmpwizard closed this Dec 7, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants