Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ajaxPostTimeout isn't properly respected in server-side request handling #1617

Closed
Shadowfiend opened this issue Aug 21, 2014 · 9 comments
Closed
Milestone

Comments

@Shadowfiend
Copy link
Member

While investigating #1614, I uncovered a mistake in the retry handling code: we actually multiply the ajaxPostTimeout by 1000 server-side, so we basically tie up a request thread per retry until the original request completes correctly, even if the original requests have been timed out correctly by the browser.

The fix for this is trivial, but it's a bug that's been around since the 2.5 release without complaint, so I'm thinking it may be worth letting 2.6 go out the door in final form given its Scala 2.11 support and such, and making the fix in Lift 3… Thoughts?

@andreak
Copy link
Member

andreak commented Aug 21, 2014

+1 for letting 2.6 go out the door and making the fix in Lift-3, and possibly backport to 2.6.1 if that ever happens.

@fmpwizard
Copy link
Member

I would normally agree with moving on to lift 3.0, but we currently have this PR
#1620

which is for 2.6 (there is also a PR with those changes for 3.0 which is already merged or about to be merged).

And then we have this one: #1613 which may be a good idea to also fix in 2.6 (I thought I saw an email or comment somewhere asking to backport it to 2.6 but can't find that email now)... found it, an email from Andreas:

Yes, setting version to 2.11.1 works, thanks a lot!
I back-ported master/diego_issue_1495 to 2.6-RC1 as I really need that fix.
TBH; I think that fix alone warrants an 2.6-RC2 after back-porting.

so how about we get all these into 2.6, while holding it for a few more weeks, but then we know lift 2.6 is that much better :)

@andreak
Copy link
Member

andreak commented Sep 4, 2014

👍

@andreak
Copy link
Member

andreak commented Sep 5, 2014

I have a patch (created with git format-patch -1 <commit_hash> for my backport of #1613 to 2.6-RC1 but don't know how to make a PR from it. Anyone care to enlighten me?

@fmpwizard
Copy link
Member

I would let Antonio or someone else confir this (I know I made a mess when I cherry-pick'ed commits from lift 2.6 to lift 3.0), but I think that in this case, what you would do is

git fetch //to get the latest, I just meeted the related PR to master (lift_30)
git checkout lift_26
// the -x adds the commit hash to the new commit messages, so we know where they are coming from
git cherry-pick -x  5e91795b4c9d07b67bfddd63fccb2813eb4e5308 4a758497a6f909b271cb74e106f8b8a8a98e4aa2
//this may produce some conflicts, after fixing them
git commit -a //and enter a message pointing to the original ticket

As far as I know, this is the correct way to backport a commit from master to a different branch, but I'd wait and see what others have to say.

once that works, you create a new branch
git checkout -b <name_here>
push it to github and use the UI to send the PR

@andreak
Copy link
Member

andreak commented Sep 6, 2014

I first cherry-picked, but as it produced conflicts I manually backported it as the changes was relatively small.
I can create a new branch branch with it, push it to github and create PR.

@Shadowfiend
Copy link
Member Author

Ok, so what's the story here? Has someone done work on this specific issue, or did we end up using this issue to discuss the merging of the others? Shall I create a PR for this particular issue?

@fmpwizard
Copy link
Member

+1 for creating the pr

On Wednesday, September 10, 2014, Antonio Salazar Cardozo <
notifications@github.com> wrote:

Ok, so what's the story here? Has someone done work on this specific
issue, or did we end up using this issue to discuss the merging of the
others? Shall I create a PR for this particular issue?


Reply to this email directly or view it on GitHub
#1617 (comment).

Diego Medina
Lift/Scala consultant
diego@fmpwizard.com
http://fmpwizard.telegr.am

@fmpwizard fmpwizard added this to the 2.6-RC2 milestone Nov 1, 2014
fmpwizard added a commit that referenced this issue Nov 1, 2014
…used as is on ScriptRenderer, so the client should just get the same value in both cases

(well, ajaxPostTimeout should be the same timeout + 500ms)

fixes #1617
@farmdawgnation
Copy link
Member

#1637 was merged. I'm closing this accordingly.

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

No branches or pull requests

4 participants