Skip to content

added a shutdown method to GeoApiContext which stops RateLimitExecutorDelayThread #261#365

Merged
domesticmouse merged 1 commit intogooglemaps:masterfrom
stafichuk:master
Nov 7, 2017
Merged

added a shutdown method to GeoApiContext which stops RateLimitExecutorDelayThread #261#365
domesticmouse merged 1 commit intogooglemaps:masterfrom
stafichuk:master

Conversation

@stafichuk
Copy link
Copy Markdown
Contributor

This is a pull request for issue #261. Details can be found in the comment but I will also copy them here for convenience.

The root cause of the issue is following. We are creating and starting a thread in a constructor of class RateLimitExecutorService. Here it is:

public RateLimitExecutorService() {
    setQueriesPerSecond(DEFAULT_QUERIES_PER_SECOND);
    Thread delayThread = new Thread(this);
    delayThread.setDaemon(true);
    delayThread.setName("RateLimitExecutorDelayThread");
    delayThread.start();
}

run method of this class looks like this:

@Override
public void run() {
  try {
    while (!delegate.isShutdown()) {
      this.rateLimiter.acquire();
      Runnable r = queue.take();
      delegate.execute(r);
    }
  } catch (InterruptedException ie) {
    LOG.info("Interrupted", ie);
  }
}

There are two problems with this method:

  1. RateLimitExecutorDelayThread should end when the delegate is shut down but the thing is RateLimitExecutorService#shutdown method and thus shutdown() method of the delegate is never called.
  2. If we call shutdown() method but the queue is empty we will wait forever in queue.take().

This issue manifests itself when you are restarting an application multiple times inside a single JVM without restarting JVM itself. For example, it can happen in application servers like Tomcat or in Play Framework's dev mode, which was my case.

@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@stafichuk
Copy link
Copy Markdown
Contributor Author

I signed it!

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

@stafichuk stafichuk changed the title fixed thread leak #261 added a shutdown method to GeoApiContext which stops RateLimitExecutorDelayThread #261 Nov 5, 2017
@domesticmouse domesticmouse merged commit d865b9d into googlemaps:master Nov 7, 2017
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