Skip to content

➰ fixed long-running threads#21

Merged
markmcd merged 4 commits intogooglemaps:masterfrom
markmcd:threadfix
Sep 15, 2014
Merged

➰ fixed long-running threads#21
markmcd merged 4 commits intogooglemaps:masterfrom
markmcd:threadfix

Conversation

@markmcd
Copy link
Copy Markdown
Contributor

@markmcd markmcd commented Sep 12, 2014

PTAL @broady @domesticmouse

I did some testing using a very basic sample app and it wasn't closing after main() finished. This changed value is passed to setDaemon(), so the thread should be closed.

@broady
Copy link
Copy Markdown
Contributor

broady commented Sep 12, 2014

Hmmm — we don't really need this for the async requests, just the sync ones, right?

But to make that happen, we'd need two ExecutorService objects, yeah?

@markmcd
Copy link
Copy Markdown
Contributor Author

markmcd commented Sep 12, 2014

Not sure I understand. Sync requests are turned into async requests and we just block until they're finished, so this is used for both.

Or do you mean we shouldn't daemonize requests that are executed async? We could parameterise the daemon flag and set based on async or not...

@broady
Copy link
Copy Markdown
Contributor

broady commented Sep 12, 2014

Or do you mean we shouldn't daemonize requests that are executed async? We could parameterise the daemon flag and set based on async or not...

Correct. I don't think we can parameterise it though, since it's set when RateLimitExecutorService is constructed. Let's leave as-is for now, but just add a comment with an explanation.

@markmcd
Copy link
Copy Markdown
Contributor Author

markmcd commented Sep 12, 2014

Done.

@broady
Copy link
Copy Markdown
Contributor

broady commented Sep 12, 2014

I just noticed the Util class is under okhttp.internal. Can we just copy and paste it so we're not using an internal class?

@markmcd
Copy link
Copy Markdown
Contributor Author

markmcd commented Sep 12, 2014

Done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dd

@broady
Copy link
Copy Markdown
Contributor

broady commented Sep 15, 2014

LGTM!!

markmcd added a commit that referenced this pull request Sep 15, 2014
➰ fixed long-running threads
@markmcd markmcd merged commit 1b51b56 into googlemaps:master Sep 15, 2014
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.

2 participants