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

Re-loggin in if session token is close to expiring #735

Merged
merged 6 commits into from
Jul 25, 2016

Conversation

MikeDX
Copy link
Contributor

@MikeDX MikeDX commented Jul 25, 2016

Short Description:

Check for stale login and re-login if necessary

Fixes:

Bot working on "nothing" after a while due to session expiry

@Nihisil
Copy link
Contributor

Nihisil commented Jul 25, 2016

I hope it will work as described 👍

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

It should do, I've done all the sanity checks, just letting it run for a while now to see if it does or not...

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

This failed for some reason, and I'm now trying to work out why, so please don't merge (for now)

@solderzzc
Copy link
Contributor

sure.

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

A change is needed in the pgoapi.py file. PokemonGo-Map has a different file to us. Since we use a cloned version and not our own, any idea of the best way to go about changing it?

The second login is failing because the RPC endpoint is not returned for the second login. PokemonGo-Map either uses a different branch to us or a custom file (I can't tell which since they have the API folder included in their git repo and not sourced)

@Nihisil
Copy link
Contributor

Nihisil commented Jul 25, 2016

Are you sure, that it can't be fixed without changes in pgoapi? For example you can try to reinit API on login, or you can try to set api._auth_token = None. Something like this.

@bool-
Copy link

bool- commented Jul 25, 2016

It appears the relevant changes are here. AHAAAAAAA/PokemonGo-Map@c24590e

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

You are both correct

@bool- you confirmed what I was trying to change

@Nihisil I have reset the api, and this appears to work!

Pushing new changes now

@dddbliss
Copy link
Contributor

+1 on this.

@Nihisil
Copy link
Contributor

Nihisil commented Jul 25, 2016

I tested it a little bit, and it seems like to be a working solution 👍

[2016-07-25 10:46:19] [#] Attempting login to Pokemon Go.
[2016-07-25 10:46:24] [+] Login to Pokemon Go successful.
...
[2016-07-25 11:44:34] Session stale, re-logging in
[2016-07-25 11:44:34] [#] Attempting login to Pokemon Go.
[2016-07-25 11:44:37] [+] Login to Pokemon Go successful.
[2016-07-25 11:45:20] [#] Something rustles nearby!

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

Thanks @Nihisil for the tip about resetting the API. That was a much better solution than hacking the pgoapi (which imho is broken when trying to get a new token, but there we go) :)

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

Mine has been running for 4 hours now and still farming away!

@solderzzc
Copy link
Contributor

can you have the conflict fixed ?

@tonz134
Copy link

tonz134 commented Jul 25, 2016

This works like a charm.
been on for 3 hours now

@myeunggithub
Copy link

myeunggithub commented Jul 25, 2016

Just forked the new __init__.py file to my bot, will report back on results.

@solderzzc
Copy link
Contributor

This branch has conflicts that must be resolved ....... oops, duplicated review....... I need this merged badly.

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

Didn't have any conflicts when I submitted it!!! :(

Fine I'll go fix it :P

Conflicts:
	pokemongo_bot/__init__.py
@skydrome
Copy link

working here, thanks for this much needed fix!

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

ok this has the conflicts resolved, but I've no idea what the travis build errors are for/from.

@tstumm
Copy link
Contributor

tstumm commented Jul 25, 2016

Can you move the session expiration check to a separate function?

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

I can, but I will wait until this branch is fixed before I do so, otherwise I will probably end up with more merge conflicts.

Actually, I don't see much point in moving a single line if statement to a function, what is your thinking behind this? Do you want to get the session time elsewhere? Or do you mean the check and re-login?

@tstumm
Copy link
Contributor

tstumm commented Jul 25, 2016

Well we don't like monolithic code. It just makes the code more readable and more clear.

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

Ok, well, as I asked before - can you clarify what part you want? Just the call to get the remaining_time value or the whole section from the first if check down to login?

@tstumm
Copy link
Contributor

tstumm commented Jul 25, 2016

        # Check session expiry
        if self.api._auth_provider and self.api._auth_provider._ticket_expire:
            remaining_time = self.api._auth_provider._ticket_expire/1000 - time.time()

            logger.log("Session stale, re-logging in", 'yellow')
            self.login() 

That part.

@douglascamata
Copy link
Member

douglascamata commented Jul 25, 2016

@MikeDX would you want to extract login logic out of the Bot class and into a worker, just like all the others?

@douglascamata
Copy link
Member

Waiting code isolation into separate worker to merge.

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

I've moved the login check out of the work_on_cell function and moved it to its own method.

This has gone from being a nice easy fix to being a real time consuming exercise 👎

@douglascamata
Copy link
Member

@MikeDX I know, but it's for the best of the codebase, because we're moving towards behaviour trees and we need to make code more modular. Even though you didn't do as I suggested, that's no problem because now it's easy for me to do it myself.

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

@douglascamata I'm happy for you to do it, I'm still not 100% up to speed with the whole new refactoring of the project, and having that horrible broken commit (with the teleporting) really put me off - felt like untested code was getting merged and useful code that prevent actual crashes like this was being nitpicked ;)

@douglascamata
Copy link
Member

@MikeDX it's more like we have too many different situations to test code :/

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

I can appreciate that.

There are two times I can tell code has been merged with no apparent testing

  • LCD support
  • walker / teleporting (testing this on anything beyond seeing if it compiles would have thrown the error everyone reported)

I am sure there have been others.

This fix I did originally was purely meant to satisfy a problem everyone was having - that the bot was dying after ~one hour, I wasn't prepared for the extra merge fixes (understandable) and subsequent refactoring.

I want to contribute to this project and have many ideas for improvements, but if the testing / approval / merge process is as shabby as this, it's just going to be a frustrating experience, which nobody wants.

@douglascamata
Copy link
Member

@MikeDX it will become better when behaviour tree lands. With isolation code will become easier for us to review. Right now ppl are always trying to add too many features inside a method that they become bloated and errors are hard to spot.

@douglascamata douglascamata merged commit 1312ca4 into PokemonGoF:dev Jul 25, 2016
@LasseSkogland
Copy link

Typo in code pokemongo_boy/__init__.py
On line 73 change location to position.

@MikeDX
Copy link
Contributor Author

MikeDX commented Jul 25, 2016

Oh for goodness sake.

Why the hell is it location in take_step and position in work_on_cell

This codebase is just a total mess.

Guess I"ll go and make a new pull request...

MFizz pushed a commit to MFizz/PokemonGo-Bot that referenced this pull request Jul 29, 2016
* Attempting to solve the stale token issue by re-issuing if the token has less than 60 seconds until expiry

* Fixes remaing time check

* Reset pgoapi when rpc token is stale

* Refactored session expiry check
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.