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

Thread Per User #2956

Merged
merged 18 commits into from
Aug 1, 2016
Merged

Thread Per User #2956

merged 18 commits into from
Aug 1, 2016

Conversation

chuyskywalker
Copy link
Contributor

@chuyskywalker chuyskywalker commented Aug 1, 2016

Description

I have no doubt that we're never getting multi-threading per account back. We can, however, do a better job at threading across multiple accounts.

This does that, and cleans up some other things related.

Motivation and Context

Wanted to fix a bug wherein the searcher pause would not return after a while. Realized I need to move the PgoAPI waaay deeper into the system to make that work. Things snowballed a little bit, but I made a lot of, what I consider, to be good improvements.

  • BREAKING: interactive password prompt removed
  • Removes "thread_delay" as it doesn't really serve a purpose
  • Removed "num_threads" since it's now directly tied to number of accounts
  • -u/-p/-a parsing is handled the same, but with better error messages and better intenral list handling
  • Removed lat/lon global state from config
  • Fixed error where turning off scanning for long periods of time would fail to restart the scanner
  • Removed a bit of dead code
  • search_overseer now creates threads as needed
  • "next location" is no longer handled through global config
  • "next location" is handled more gracefully (search_threads no longer aware of it)

How Has This Been Tested?

Not very extensively yet. In fact, I accidentally (and sorta stupidly) built it from #2863, so it looks like a lot more code than it really is. That other PR should get merged first; then this one will look less all over the place.

That all said, this needs a lot more attention for all the various modes and methods we support. (Unit/Integration testing? What's that!?)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

(Doc updates will definitely be in order with this one -- but there, generally speaking, seems to be a huge need for "HEY, SCANNING WORKS LIKE XYZ NOW, THINGS CHANGED" type announcement.

 - BREAKING: interactive password prompt removed
 - Removes "thread_delay" as it doesn't really serve a purpose
 - Removed "num_threads" since it's now directly tied to number of accounts
 - `-u/-p/-a` parsing is handled the same, but with better error messages and better interal list handling
 - Removed lat/lon global state from `config`
 - Fixed error where turning off scanning for long periods of time would fail to restart the scanner
 - Removed a bit of dead code
 - search_overseer now creates threads as needed
 - "next location" is no longer hanlded through global config
 - "next location" is handled more gracefully (search_threads no longer aware of it)
@powerpatch
Copy link

powerpatch commented Aug 1, 2016

I'd love to give this a test I'm currently using a different one but it has a few hiccups.
I'm a little new to github what's the easiest way for me to implement this besides copy/pasting the code in myself?

EDIT: also what syntax are you using for the multiple accounts?

@chuyskywalker
Copy link
Contributor Author

git checkout -b chuyskywalker-thread-per-user develop
git pull https://github.com/chuyskywalker/PokemonGo-Map.git thread-per-user

Then rebuild and run it.

@powerpatch
Copy link

Not sure what the problem is but just like the latest develop build as soon as I execute runserver it closes.

@chuyskywalker
Copy link
Contributor Author

It should give you some output. Add -d if not. That said, if it's happening on current develop this thread may not be the right place to discuss the issue.

@powerpatch
Copy link

powerpatch commented Aug 1, 2016

I guess the one click install is broken right now.
I got it working but now I need to know how to use the multiple accounts.
edit: Got it all figured out giving it a good test now thank you 👍

@DomGrieco
Copy link

@chuyskywalker This is great stuff. I'll set it up now and let it run overnight.

@Ashex
Copy link
Contributor

Ashex commented Aug 1, 2016

If we're now assigning a user per thread, is there any way to mark this in the database so that multiple workers can be passed the same set of credentials and they won't reuse them?

@powerpatch
Copy link

powerpatch commented Aug 1, 2016

somanyrares
using #2546
2016-07-31 20:44:48,848 [ search] [ INFO] Search loop 5 starting
2016-07-31 20:49:02,499 [ search] [ INFO] Search loop 5 complete.
4m14s
using #2956 (this one)
2016-08-01 01:00:38,621 [ search_worker_1][ search][ INFO] Searching step 1, remaining 90
2016-08-01 01:04:45,038 [ search_worker_1][ search][ INFO] Searching step 91, remaining 0
4m7s

I'm using a very large number of accounts -st 6 with 3 on each instance.
Sometimes it takes longer but I'll blame the database locks and pgo servers for that for now. (any suggestions? #2953)
The way this is handling multiple accounts is amazing because it doesn't lock the whole window waiting on other workers issues.
Since I started not a single one has crashed or become locked up for a period of time which is a major issue with the other one.
This handles threads/problems very gracefully and I hope to see it implemented soon! 👍
It also seems to be very accurate considering I have never seen more than 5 of my filtered list and in the first hour of running this I'm seeing 8 already. (one missed the screenshot oops)

@Frozen-byte
Copy link

Obviously I am doing something wrong?
Yet I am getting this error message just after starting my Script:

2016-08-01 00:05:35,815 [      MainThread][        models][    INFO] Connecting to MySQL database on localhost
2016-08-01 00:05:35,824 [   search_thread][        search][    INFO] Search overseer starting
2016-08-01 00:05:35,824 [   search_thread][        search][    INFO] Starting search worker threads
Exception in thread search_thread:
Traceback (most recent call last):
  File "/usr/lib/python2.7/threading.py", line 551, in __bootstrap_inner
    self.run()
  File "/usr/lib/python2.7/threading.py", line 504, in run
    self.__target(*self.__args, **self.__kwargs)
  File "/root/PokemonGo-Map/pogom/search.py", line 121, in search_overseer_thread
    t.start()
  File "/usr/lib/python2.7/threading.py", line 494, in start
    _start_new_thread(self.__bootstrap, ())
error: can't start new thread

Defined my Users in the config.ini:

username: [account1, account2, account3]
password: samepasswordforallaccounts

Started with the following:

./runserver.py -l "location" -st 50

Frozen-byte referenced this pull request Aug 1, 2016
* Add support for multiple accounts (#2546)

Changes program arguments:

-u/--user and -p/--password have been removed.

-a/--auth has been changed to -a/--auths and
takes in one or more strings of form:
auth_provider:username:password

auth_provider is ptc or google.

If there are n accounts, search thread i will use account (i % n).

* Fix moving credentials to lowercase

* Add backwards compatibility to multiple accounts

Multiple accounts are no longer given as strings containing
auth service, username and password, because that was not
backwards compatible.
Now -a, -u and -p each can be given multiple times.

With this patch multiple usernames can be given.
They either use one password for all accounts or
unique password for each account can be passed.
Same goes for auth services. Auth defaults to ptc.

Unfortunately the way argparse handles config files
each these cannot take multiple parameters.
'-u user1 user2' is not possible, needs to be
'-u user1 -u user2'

The following parameters should be ok:

Using single account should be just as it was earlier:
./runserver.py -a ptc -u user -p pass -st 3 -t 3 ...

This will use ptc for all accounts and ask interactively
for one password to use for all accounts:
./runserver.py -u user1 -u user2 -u user3 -st 3 -t 3 ...

This will use password 'pass' for all accounts and use ptc:
./runserver.py -u user1 -u user2 -u user3 -p pass -st 3 -t 3 ...

This will use google for all accounts and unique passwords
for each account:
./runserver.py -a google -u user1 -u user2 -p pass1 -p pass2 ...

This will log one in ptc and one in google with respective un/pw:
./runserver.py -a ptc -a google -u user1 -u user2 -p pass1 -p pass2 ...

In config.ini these can be given as lists or single values:
auth-service: [google, ptc]
username: [user1, user2]
password: onepasswordforbothaccounts

* Add support for multiple accounts (#2546)

Changes program arguments:

-u/--user and -p/--password have been removed.

-a/--auth has been changed to -a/--auths and
takes in one or more strings of form:
auth_provider:username:password

auth_provider is ptc or google.

If there are n accounts, search thread i will use account (i % n).

* Fix moving credentials to lowercase

* Add backwards compatibility to multiple accounts

Multiple accounts are no longer given as strings containing
auth service, username and password, because that was not
backwards compatible.
Now -a, -u and -p each can be given multiple times.

With this patch multiple usernames can be given.
They either use one password for all accounts or
unique password for each account can be passed.
Same goes for auth services. Auth defaults to ptc.

Unfortunately the way argparse handles config files
each these cannot take multiple parameters.
'-u user1 user2' is not possible, needs to be
'-u user1 -u user2'

The following parameters should be ok:

Using single account should be just as it was earlier:
./runserver.py -a ptc -u user -p pass -st 3 -t 3 ...

This will use ptc for all accounts and ask interactively
for one password to use for all accounts:
./runserver.py -u user1 -u user2 -u user3 -st 3 -t 3 ...

This will use password 'pass' for all accounts and use ptc:
./runserver.py -u user1 -u user2 -u user3 -p pass -st 3 -t 3 ...

This will use google for all accounts and unique passwords
for each account:
./runserver.py -a google -u user1 -u user2 -p pass1 -p pass2 ...

This will log one in ptc and one in google with respective un/pw:
./runserver.py -a ptc -a google -u user1 -u user2 -p pass1 -p pass2 ...

In config.ini these can be given as lists or single values:
auth-service: [google, ptc]
username: [user1, user2]
password: onepasswordforbothaccounts

* Merge fix
@Shentang
Copy link

Shentang commented Aug 1, 2016

I went from 10minutes logging in to 3 seconds logging in. Big thanks, enormous improvement, for the first time I feel like the old maps are actually back. I hope this gets merged soon!

@invisiblek
Copy link
Contributor

i need this in my life

@Ashex
Copy link
Contributor

Ashex commented Aug 1, 2016

How are multiple accounts defined in config.ini? The changes to utils.py appear to only support this via arguments.

@chuyskywalker
Copy link
Contributor Author

@Frozen-byte

error: can't start new thread

I don't know why that would happen. It would seem python/environment specific, since that's an internal function which is erroring out. See if google leads you down any paths.

@Ashex

It's config supported the same as the current develop branch -- didn't change how it works, Frozen left an example a few comments up.

@archalias -- thanks for testing; I'm glad to see it's just working smoother. This patch won't make the looping go (generally) any faster, as you've seen. It does fix the "everyone has to login first" startup problem though.

@Frozen-byte
Copy link

@chuyskywalker Yes I observed that this error only gets thrown when I have an high amount of Accounts (200+) I guess my Security settings preventing the additional Thread.

@chuyskywalker
Copy link
Contributor Author

The other PR got merged, so the file diff on this one now makes sense, if any one wishes to review it with clarity

@DionVermeulen
Copy link

but with this you can't set multiple workers because the other worker uses also the config.ini with the same accounts!
Or can we put the accounts in the command line?

@chuyskywalker
Copy link
Contributor Author

You can use different accounts per script invocation.

@chuyskywalker
Copy link
Contributor Author

This isn't the right thread to look for support, please.

@tarthim
Copy link

tarthim commented Aug 1, 2016

Checked this out, running with (only?) 10 accounts at the moment, but holy hell this is fast and super accurate (as far as I can test this). Great stuff. Good job.

@eluz1ve
Copy link

eluz1ve commented Aug 1, 2016

When I start scanner with 50+ accounts (1 string), 10% of accounts can't login.
ERROR] Failed to login to Pokemon Go. Trying again in 5 seconds
ERROR] Could not retrieve token: As a security measure, your account has been disabled for 15 minutes because you have failed to log in correctly too many times. Please come back and try again in 15 minutes.

Maybe add some delay between login period?

But the other things - working like a charm!

@Benthem
Copy link

Benthem commented Aug 1, 2016

Then view it as not asking for advice but as putting to light an existing issue. It's an immediate result of the many threads requiring write access.

@AHAAAAAAA
Copy link
Owner

+1, just need to update the documentation and config.example.ini files!

@DomGrieco
Copy link

@invisiblek Let's get this +1 and merged!

@invisiblek invisiblek merged commit cea85c8 into AHAAAAAAA:develop Aug 1, 2016
@ffittschen
Copy link

@chuyskywalker I know that this PR is already merged, but I have a question regarding the change:

Is this PR adding the threads dynamically based on the accounts that were able to log in or at startup time based on the number of provided accounts?
Because I'm curious if I can avoid the PTC/google login issues by passing a handful accounts of each.

It would be awesome if it scans with all google accounts, even if the PTC login is currently unavailable 😉

@Mr-Groch
Copy link
Contributor

Mr-Groch commented Aug 1, 2016

There is a bug - current_location for Flask is not updating after location change, so after page refresh, marker will return to origin. I've made simple fix in #2980

@hodoliiiii
Copy link

hodoliiiii commented Aug 1, 2016

This is not suitable for windows, is it?
In Models.py you wrote: !/usr/bin/python
I just commented it and started runserver.py. However the map is accessable but no pokemon will be detected and the cmd throws the following error msgs:

2016-08-01 22:05:42,246 [search_worker_18][        search][    INFO] Searching step 302, remaining 839
2016-08-01 22:05:42,480 [search_worker_18][        models][    INFO] Upserted 0 pokemon, 0 pokestops, and 0 gyms
[2016-08-01 22:05:42,796] ERROR in app: Exception on /raw_data [GET]
Traceback (most recent call last):
  File "C:\Python27\lib\site-packages\flask\app.py", line 1988, in wsgi_app
    response = self.full_dispatch_request()
  File "C:\Python27\lib\site-packages\flask\app.py", line 1641, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "C:\Python27\lib\site-packages\flask\app.py", line 1544, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "C:\Python27\lib\site-packages\flask\app.py", line 1639, in full_dispatch_request
    rv = self.dispatch_request()
  File "C:\Python27\lib\site-packages\flask\app.py", line 1625, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "F:\PoGo\PokemonGo-Map-develop\PokemonGo-Map-develop\pogom\app.py", line 87, in raw_data
    d['pokemons'] = Pokemon.get_active(swLat, swLng, neLat, neLng)
  File "F:\PoGo\PokemonGo-Map-develop\PokemonGo-Map-develop\pogom\models.py", line 96, in get_active
    p['pokemon_name'] = get_pokemon_name(p['pokemon_id'])
  File "F:\PoGo\PokemonGo-Map-develop\PokemonGo-Map-develop\pogom\utils.py", line 248, in get_pokemon_name
    return i8ln(get_pokemon_data(pokemon_id)['name'])
  File "F:\PoGo\PokemonGo-Map-develop\PokemonGo-Map-develop\pogom\utils.py", line 243, in get_pokemon_data
    with open(file_path, 'r') as f:
IOError: [Errno 2] No such file or directory: 'F:\\PoGo\\PokemonGo-Map-develop\\PokemonGo-Map-develop\\static/data\\pokemon.min.json'
2016-08-01 22:05:42,796 [       Thread-34][           app][   ERROR] Exception on /raw_data [GET]
Traceback (most recent call last):
  File "C:\Python27\lib\site-packages\flask\app.py", line 1988, in wsgi_app
    response = self.full_dispatch_request()
  File "C:\Python27\lib\site-packages\flask\app.py", line 1641, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "C:\Python27\lib\site-packages\flask\app.py", line 1544, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "C:\Python27\lib\site-packages\flask\app.py", line 1639, in full_dispatch_request
    rv = self.dispatch_request()
  File "C:\Python27\lib\site-packages\flask\app.py", line 1625, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "F:\PoGo\PokemonGo-Map-develop\PokemonGo-Map-develop\pogom\app.py", line 87, in raw_data
    d['pokemons'] = Pokemon.get_active(swLat, swLng, neLat, neLng)
  File "F:\PoGo\PokemonGo-Map-develop\PokemonGo-Map-develop\pogom\models.py", line 96, in get_active
    p['pokemon_name'] = get_pokemon_name(p['pokemon_id'])
  File "F:\PoGo\PokemonGo-Map-develop\PokemonGo-Map-develop\pogom\utils.py", line 248, in get_pokemon_name
    return i8ln(get_pokemon_data(pokemon_id)['name'])
  File "F:\PoGo\PokemonGo-Map-develop\PokemonGo-Map-develop\pogom\utils.py", line 243, in get_pokemon_data
    with open(file_path, 'r') as f:
IOError: [Errno 2] No such file or directory: 'F:\\PoGo\\PokemonGo-Map-develop\\PokemonGo-Map-develop\\static/data\\pokemon.min.json'
2016-08-01 22:05:43,892 [search_worker_20][        search][    INFO] Searching step 303, remaining 838
2016-08-01 22:05:44,098 [search_worker_20][        models][    INFO] Upserted 0 pokemon, 0 pokestops, and 0 gyms

What am I doing wrong?

Edit: If I first start the normal developement branch and afterwards the customized (by this thread) all pokemon will be visable on one map(of both versions).
Do I get it right that this code is now included to dev branch? Sorry I am absolutely new to github <:

@neutron666
Copy link

I'm getting a "could not decode JSON" while trying to log in ...

@chuyskywalker chuyskywalker deleted the thread-per-user branch August 2, 2016 01:48
@chuyskywalker
Copy link
Contributor Author

@ffittschen

Is this PR adding the threads dynamically based on the accounts that were able to log in or at startup time based on the number of provided accounts?
Because I'm curious if I can avoid the PTC/google login issues by passing a handful accounts of each.

I believe what should happen is that the, let's say, PTC account thread(s) will be stuck in a loop trying to login constantly. The Google account thread(s) would continue to peel search items off the list. However, some of the items would be stuck in the PTC thread(s) so the queue would never reach zero. Thus the overseer could never "refill" the queue with the search areas again.

Adding a "max tries before we give it back" option would be a good idea if there is more than one search worker thread. That'd need to come in another PR down the road though.

@chuyskywalker
Copy link
Contributor Author

@hodoliiiii run grunt build every time your update the develop branch -- you're missing a file. The exception message says this very thing if you read more closely.

@Shentang
Copy link

Shentang commented Aug 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.