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

#283 fix: Improvement of AppiumDriverLocalService for the multithreading. #291

Merged
merged 5 commits into from
Jan 14, 2016
Merged

#283 fix: Improvement of AppiumDriverLocalService for the multithreading. #291

merged 5 commits into from
Jan 14, 2016

Conversation

TikhomirovSergey
Copy link
Contributor

#283 fix Changes:

  • Work with java.util.concurrent.locks.ReentrantLock was incorect. It was causing deadlocks/hangings in multithreading. Now it is worked out and uses "fair" synchronization.
  • Improvement of the service stopping.
  • New tests were provided.

@TikhomirovSergey
Copy link
Contributor Author

@Jonahss @bootstraponline
Please take a look at this before it will have been merged.
@saikrishna321 You are invited too. :) I've used your sample code with this (saikrishna321/TestNGParallelThreads#2, it is not necessary to merge it) little improvement for the stress testing. It was green!

@@ -44,7 +44,7 @@
private final String ipAddress;
private final long startupTimeout;
private final TimeUnit timeUnit;
private final ReentrantLock lock = new ReentrantLock();
private final ReentrantLock lock = new ReentrantLock(true);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment here mentioning true means fair ordering policy? It's not obvious what true does in the absence of a comment.

@bootstraponline
Copy link
Member

Code looks good to me. It's great you have tests!

TikhomirovSergey added a commit that referenced this pull request Jan 14, 2016
#283 fix: Improvement of AppiumDriverLocalService for the multithreading.
@TikhomirovSergey TikhomirovSergey merged commit d6270ae into appium:master Jan 14, 2016
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