GeoApiContext should clean up working threads on exit. #261
Comments
|
The simplistic answer is that your server is running out of RAM. You probably need to run your server with some diagnostic logging to figure out if you are leaking resources somewhere. A temporary stopgap would be to increase the amount of memory available to the process. |
|
@domesticmouse However, today I watched the JVM with VisualVM and I get a lot of RateLimitExecutorDelayThread that stills on "PARK". Thats happens on Undertow and Jetty.
Seeing #232 and #207 , to me it looks like it has some problem that involves the thread. Do you have any suggestion? |
|
I don't have any immediate fixes, but I would ask you to test the latest version as I think we may have made some changes between .17 and .20 that may impact this behavior. My understanding of #232 is that we are merely seeing that the OkHttp thread isn't being closed out on shutdown. And I'm unsure how #207 is akin to what you are currently seeing. My suspicion is that you need to up the number of threads your system is configured to allow, or spread the workload over more machines. |
|
This memory leak is still present in version .20. :/ The opened threads didn't close. They are still active after all my google maps requests are done. In the attached screenshot there are 100 requests. All threads are stille open. If the number of requests are increased the out of memory exception because of the active threads will be thrown. |
|
I solve that implementing a Singleton to GeoApiContext instance
…On May 31, 2017 12:20, "Markus Kühle" ***@***.***> wrote:
This memory leak is still present in version .20. :/
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#261 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABxiwRF-6Sjh1wJWtoWwHaGEaOq6ZiNuks5r_YVUgaJpZM4NCdZd>
.
|
domesticmouse
changed the title
|
Re-opening to give myself a reminder that I need to fix this errant behaviour. |
|
This will be fixed in the next release version |
|
Many thanks for fixing this. We have been fighting with that also. Can you guys tell when the next release is due? I am kind of waiting for this fix. |
|
Release is in process. Should turn up on maven in about a day. |
|
This is still an issue as of version 0.2.4 using the Ok HTTP request handler. Use a singleton google maps API configuration singleton as a workaround. |
|
I'm also facing this issue. The root cause 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:
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. |
added a shutdown method to GeoApiContext which stops RateLimitExecutorDelayThread #261
|
Just to let you know - i'm using |
|
@domesticmouse I also see one more about |
|
Here's my whole stacktrace: |
|
Version 0.2.8 is in the process of being uploaded through maven, tho I doubt it changes the status of the error you are seeing. I'll look into this once I get some time. Thanks for the report! |
|
Question: my impression from reading the code base was that GeoApiContext was intended to be used as a Singleton, or at least one instance per account/API key, otherwise the rate limiting wouldn’t work correctly, since Google’s rate limiting aggregates queries on a per-account or per-key basis. Are you expecting to have multiple GeoApiContext objects active simultaneously? @pooi-pooi: I don’t see a shutdown() call in your example code. Am I missing something? I think you will have to call shutdown regardless of how the APi is defined, since finalizers can’t be relied on. |
|
Yup it is meant to be a singleton. There are advanced use cases where
multiple contexts make sense, thus the lack of language level enforcement
of the singleton pattern.
…On Mon, 2 Jul 2018 at 3:38 pm, Andrew Janke ***@***.***> wrote:
Question: my impression from reading the code base was that GeoApiContext
was intended to be used as a Singleton, or at least one instance per
account/API key, otherwise the rate limiting wouldn’t work correctly, since
Google’s rate limiting aggregates queries on a per-account or per-key
basis. Are you expecting to have multiple GeoApiContext objects active
simultaneously?
@pooi-pooi <https://github.com/pooi-pooi>: I don’t see a shutdown() call
in your example code. Am I missing something? I think you will have to call
shutdown regardless of how the APi is defined, since finalizers can’t be
relied on.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#261 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAB3Jy7DOupLJdj1xgXM_N16RG5n7m0Iks5uCbHCgaJpZM4NCdZd>
.
|
|
Here's another data point. I've added The
The
(BTW, ha ha, isn't it cute how JConsole's window is a picture of a Mac window UI with its own title bar and window chrome?) And here's the multi-threaded "dirty" result, where the worker threads do not call shutdown().
That hit my rate limit pretty quick. If you do a bunch of short-lived worker threads that only do a few queries each, and fail to call shutdown(), then it's off to the races. This would be more like the scenario of a server process that launches a new worker and a new
With shutdown() calls, it steadies out at about 500 threads for me.
While this didn't even bring my desktop to my knees, it does look like a thread leak. Skimming through the source, I don't see anything that's actively managing a worker pool size. It looks like RateLimitExecutorService is just creating fresh threads for queries as they come in. Am I missing something? |
I totally misread the source; it is in fact using a worker pool. @ptahchiev I'm unable to reproduce the leak of threads from the main google-maps-services-java classes. Are you sure your app is calling I am seeing thread growth up to about a steady state of 800 threads when I run a bunch of multi-threaded queries using their own GeoApiContext objects under the current code.
But looking at it in VisualVM, they all seem to be OkHttp worker pool threads. And they do seem to get cleaned up once there's some memory pressure happening. Plus, once I stop issuing new queries, they eventually all go away on their own.
This was after 1000 separate GeoApiContexts were built and used. So I don't think there's a thread leak in google-maps-services-java per se any more, at least under simple usage scenarios. Maybe a finalizer should be added to GeoApiContext to help support the scenario where the application is shut down from the outside by a web service container? |
|
This is still an issue, we get it in a few applications and causes Tomcat to crash |
|
@barrychapman which version of the library are you using? |
|
Sorry for the delay. |
|
Some of the errors in The -only- place in my application that RateLimitExecutorService is invoked is inside the GoogleApiContext file. |
jpoehnelt
added
triage me
|
Getting the same issue :( |
|
Exception in thread "Rate Limited Dispatcher" java.lang.OutOfMemoryError: unable to create new native thread |
|
Apparently if you are going to do this in your code like the docs specify: You must also do this: Which the docs do not specify. This caused us to take major outages today as it would creep up under heavy workloads that did not surface during our regular testing. Here are the misleading docs
Most of us like TLDR; not sure where the context.shutdown(); requirement is hidden in the docs. Major pain in my ass. Thank you so much Google maps team. @thpoiani's solution is what saved us in the end. Thanks for the recommendation. For example, if you have a Spring Boot app, you should do this in one of you configuration classes: And in the class where you want to issue a GeocodeAPI call, do this to get a reference to the singleton: What the code should instead provide is an out of the box singleton implementation which prevents this issue from happening (ties up a single thread eternally, instead of eventually causing the server to run out of threads). What should also happen is that the docs should clearly state that the wheels of your server are going to come off if you do not also do a context.shutdown() if the above supplied code is going to be continuously called. |
|
Version 1.0.1 is still suffering a thread leak. Even using the GeoApiContext as a singleton, a "Rate Limited Dispatcher" thread is created for each call and not cleared up (there is however only one RateLimitExecutorDelayThread) I have had to resort to creating and destroying the GeoApiContext everytime we call the API, in a try-with-resources statement to get around this problem. This doesn't seem like a very efficient solution: a thread dump shows all the Rate Limited Dispatcher threads as follows: |
|
I noticed a problem when instantiating GeoApiContext.Builder. I want to use my own RequestHandler.Builder. So I used the This creates threads that are never closed.
When we then call the The When the The Threads created by After a while, I get the following exception: A workaround is to not use the I see 2 possible solutions to correct the problem:
|
Hello guys, I had a problem while doing a PlacesApi.placeAutocomplete request
I really don't know if that is a issue caused by the API or on our implementation.
We are developing an application that uses:
1.3.6.RELEASEUndertowcom.google.maps:google-maps-services:0.1.17Sometimes we get this exception:
Stacktrace:
stacktrace.txt
Do you know what could causes that?
The text was updated successfully, but these errors were encountered: