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

Make ApplicationProvider.get asynchronous #7645

Open
richdougherty opened this issue Jul 18, 2017 · 5 comments
Open

Make ApplicationProvider.get asynchronous #7645

richdougherty opened this issue Jul 18, 2017 · 5 comments
Assignees

Comments

@richdougherty
Copy link
Member

There have been a number of issues caused by the blocking nature of ApplicationProvider.get: #5975, #7614, #7622. Threads get starved because every thread can end up blocking while reloading. Only one thread really needs to block—the reloading one—and the others could just wait for the result in a Future[Application].

I'm not implementing the change yet because we're still backporting stuff to 2.6 and this seems a bit too heavy to backport. However, here's a sketch solution so it's easy to implement later:

trait ApplicationProvider {
  @deprecated
  def get: Try[Application]
  def getAsync(): Future[Application]
}
class DevServerStart {
  val appProvider = new ApplicationProvider {
    // Used for atomic locking
    private val reloadAtomic = new AtomicReference[Option[Future[Application]]](None)
    def getAsync(): Future[Application] = {
      // Use an atomic lock to restrict access so only one
      // thread reloads the application at a time.
      @tailrec def atomicLoop(): Future[Application] = {
        val atomicRead = reloadAtomic.get()
        atomicRead match {
          case Some(future) => future
          case None =>
            val promise = Promise[Future[Application]]
            val atomicWrite = Some(promise.future)
            if (currentReload.compareAndSet(None, myFuture)) {
              val reloadFuture = getAsyncExclusively()
              promise.completeWith(reloadFuture)
            } else {
              loop()
            }
        }
      }
      atomicLoop()
    }
    def get(): Try[Application] {
      val readyFuture = Await.ready(getAsync(), Duration.Inf)
      readyFuture.value.get
    }
    private def getAsyncExclusively(): Future[Application] = {
      ... buildLink.reload, etc ...
    }
  }
}
@richdougherty richdougherty added this to the 2.7.0 milestone Jul 18, 2017
@richdougherty richdougherty self-assigned this Jul 18, 2017
@richdougherty richdougherty changed the title Make ApplicationProvider.get asynchronous Make ApplicationProvider.get asynchronous Jul 19, 2017
@viktorklang
Copy link
Contributor

viktorklang commented Jul 20, 2017

@richdougherty

You could consider something like this to keep object creation and indirection down a bit:

// Used for atomic locking
    private val reloadAtomic = new AtomicReference[Promise[Application]](null)
    def getAsync(): Future[Application] = {
      // Use an atomic lock to restrict access so only one
      // thread reloads the application at a time.
      @tailrec def atomicLoop(p: Promise[Application]): Future[Application] = reloadAtomic.get() match {
        case null =>
          val mkPromise = if (p eq null) Promise[Future[Application]] else p
          if (reloadAtomic.compareandSet(null, mkPromise))
            mkPromise.completeWith(getAsyncExclusively()).future
          else atomicLoop(mkPromise)
        case some => some.future
      }
     atomicLoop(null)
  }

@richdougherty
Copy link
Member Author

OK thanks. :)

And how did you even notice this ticket, do you have an email filter scanning for the word AtomicReference? 😉

@viktorklang
Copy link
Contributor

@richdougherty The Klang sees everything.

@richdougherty
Copy link
Member Author

For those who don't know about ApplicationProvider.get, this issue only affects dev mode. It won't happen in production.

@He-Pin
Copy link
Contributor

He-Pin commented Jan 2, 2018

@viktorklang The Klang love @tailrec and AtomicReference[_].

@marcospereira marcospereira removed this from the Play 2.7.0 milestone Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants