Skip to content
This repository has been archived by the owner on Dec 15, 2018. It is now read-only.

Full refactoring #222

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

friscoMad
Copy link
Contributor

@friscoMad friscoMad commented Dec 29, 2017

This is based on #221 so it has all the changes @michikrug did.
It does break backwards compatibility in at least 2 ways:

  • No longer supports proto2dict responses
  • Hash headers are returned with each call and can not be accesed like we were doing previously.

Main changes (probably not an exhaustive list):

  • Niantic calls share a session by default this reduces the total number of open connections to niantic heavily (in my tests with RM a 145 worker setup can use as little as 30 connections) it is not perfect emulation but it is a lot more efficient and shouldn't be an issue as connections seems to be terminated in the ssl load balancer.
  • Hash engine is fully static, most of the state was already in a state class with Update everything (cleaned up) #221 so I just moved everything api related to that class.
  • PTC session is freed after login, we were clearing cookies anyway during the login so there is no reason to keep the connection objects and data open forever.
  • Added support to specify proxy and hash key per request (it was possible to be done before but not that easy).
  • Added support for changing hash endpoint (for goman hash and possible future services)
  • Removed example code that was not updated and not working
  • Removed unneeded deps
  • Fixed sig.activity_status.stationary logic
  • Removed hashing header parsing and just return them with the response, as other hashing engines can use different headers and there is no need to parse the ones that the app does not want to use.
  • Improved tipe conversion in hash response parsing, we were casting the data twice and adding it to the request in a loop instead of all at once.
  • Added adapters for other protocols in niantic calls as proxies do use the pool associated with their protocol and host so adapter configuration was not being used for proxified requests with protocol different from https:

Some thoughts and things to discuss:

  • State class parameters could just be added to api main class I opted for keeping as close it was after seb changes as it seems clean but it is another object that we will keep in memory.
  • Shared niantic session by default, I think it is ok but if we want to be real safe we can default to a session per api.
  • Adding code to use hashing endpoint based on key, this works for goman, not sure about future hashing services, so I avoided adding it to be as neutral as possible.
  • urllib3 does not clean up the connection pool (connections time out but objects are kept in memory) so single session can lead to memory "leaks" if a lot of different proxies are used. I opened the bug Connections kept forever in connection pool urllib3/urllib3#1301 but there is no clean way to handle this.

With this changes my production RM instance reduced mem comsumption from around 550Mb to 380Mb that's a 30% reduction.

I don't expect this to be merged soon but I hope we can clear out the discussion list and fix/add whatever is needed.
And I cand split this in a lot of small prs but we need to define the previous thoughts list and given the small amount of time @ZeChrales has I thought it was better to just review it all at once.

Michael Krug and others added 6 commits December 22, 2017 01:19
Removed empty interface hash_engine
Made Hash_server static no need to instantiate
Removed unused requirements
Removed dict usage

Use single pool for all rpcs
Store state in a single object and made hash_server and rpc_api static
Improved logic

Enable passing proxy and hash_key in the call

Add support for disabling session reuse and changing hashing endpoint

Don't store session in auth as it is not needed
Reduce rpc pool size to 150 (that should be enough for 500-600 workers)
Add adapter mounts for all protocols as proxies uses the pool by their own urls

Remove unused function
@michikrug
Copy link

michikrug commented Dec 30, 2017

Starting testing this right now...

First things noticed:

Breaks current usage in RM due to:

  • HashServer.status is not provided anymore for key status values
  • access to auth_provider has moved to state

@friscoMad
Copy link
Contributor Author

Yup I have a branch with needed changes for RM: https://github.com/friscoMad/RocketMap/tree/frisco-futureApi

@michikrug
Copy link

Will merge those changes into my fork and test further :)

@michikrug
Copy link

Seems to run pretty well with the mentioned RM changes

@friscoMad
Copy link
Contributor Author

I just found that I was not setting any device info in the signatures still I have been using this api for some time in my main instance and haven't seen any strange behaviour 🤷‍♂️
I will push the fix anyway.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants