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 doesn't have to be multiplied by 1000, because it is use... #1637

Merged
merged 1 commit into from
Nov 16, 2014

Conversation

fmpwizard
Copy link
Member

...d 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

…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
@Shadowfiend
Copy link
Member

Will try and give this a test spin tomorrow, but the code looks good here.

@farmdawgnation farmdawgnation modified the milestone: 2.6-RC2 Nov 11, 2014
@farmdawgnation
Copy link
Member

👀

@farmdawgnation
Copy link
Member

So, what should I do to test this, exactly? I need to create the conditions that nextAction is a Right. I'm not clear from a scan of the code how to actually do that...

@Shadowfiend
Copy link
Member

Make something that will take longer than a single AJAX cycle to compute (typically 5s I believe?) via Thread.sleep, then stick a println around line 652 so that you can see when the future times out. It should time out shortly after the corresponding AJAX request (before, it would keep running for much longer because we were multiplying the timeout by 1000).

@farmdawgnation
Copy link
Member

👀

@farmdawgnation
Copy link
Member

Ok, so I couldn't successfully see this working. Here was my testing procedure.

  • Added a println on 652 so my code pretty much looked like this:
          case Right(future) =>
            val ret = future.get(ajaxPostTimeout) openOr {
              println("ABORTING!")
              Failure("AJAX retry timeout.")
            }

            ret
  • Loaded up this project and then clicked the button which should cause the thread to sleep.

The thread does sleep, and the post timeout does occur, but I never see "ABORTING!" printed to the console. 😕

@farmdawgnation
Copy link
Member

Poke.

@Shadowfiend
Copy link
Member

I'm seeing expected behavior:

> Waiting on future for 5500
Got dat Failure(AJAX retry timeout.,Empty,Empty)
Waiting on future for 5500
Got dat Failure(AJAX retry timeout.,Empty,Empty)
Waiting on future for 5500
Got dat Failure(AJAX retry timeout.,Empty,Empty)

With:

            println(s"Waiting on future for $ajaxPostTimeout")
            val ret = future.get(ajaxPostTimeout) openOr Failure("AJAX retry timeout.")
            println(s"Got dat $ret")

Make sure you published the Scala 2.11 version locally; I initially published the 2.10 version by accident and didn't see the behavior.

👍 from me but let's let @farmdawgnation give it another shot as well.

@farmdawgnation
Copy link
Member

Make sure you published the Scala 2.11 version locally; I initially published the 2.10 version by accident and didn't see the behavior.

Maybe that's what happened. Let me try again real quick.

@farmdawgnation
Copy link
Member

Ok, I'm now seeing the expected behavior. Let's ship it. :shipit:

farmdawgnation added a commit that referenced this pull request Nov 16, 2014
Fix an issue in ajaxPostTimeout calculation in LiftServlet that caused futures to stick around spinning longer than they should have.
@farmdawgnation farmdawgnation merged commit d92a14d into lift_26 Nov 16, 2014
@farmdawgnation farmdawgnation deleted the diego_issue_1617 branch November 16, 2014 19:35
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.

3 participants