Skip to content
This repository has been archived by the owner on Jan 23, 2024. It is now read-only.

switch to ios api #97

Closed
simon-weber opened this issue Sep 10, 2016 · 15 comments
Closed

switch to ios api #97

simon-weber opened this issue Sep 10, 2016 · 15 comments

Comments

@simon-weber
Copy link
Owner

Currently, we use the web api (the one music.google.com uses). The motivation for picking it was mostly to deal with auth: we can just reuse the open tab's session.

It has a number of disadvantages:

  • syncing often doesn't work: around 25% of sync attempts fail. For some accounts, syncing just doesn't seem to work at all.
  • if the music tab gets closed, we eventually lose auth.
  • there's no support for batching operations, making syncs slower and more disruptive to the UI.
  • the serialization format is a pita.

I think the best alternative is the ios api, which I got working last year in gmusicapi but never looked into further. By comparison:

  • it supports batching arbitrary operations, hopefully improving sync speed and quality.
  • it can auth without an open tab (and doesn't have to deal with xsrf).
  • the serialization format is easy to deal with.
  • it's intended for use by a stateful client.
  • it moves oauth forward, giving an opportunity to ask for play store access (for include one week full access in trial #80).

It's a bit of a risk since Google could more easily cut off access to it, but it could potentially solve a lot of problems. Building it out and making it opt-in could be a good way to test it out.

@yinka
Copy link

yinka commented Sep 11, 2016

I'd be happy to help test this

@simon-weber
Copy link
Owner Author

I've got an iOS device now! I hope to have time for this in the next few weeks.

@rcmpayne
Copy link

I have almost every iOS device so if you need me to test anything, just let me know.

@simon-weber
Copy link
Owner Author

I took a quick look at the ios api, and it looks like they've unified it with what the Android app uses. This makes things a bit easier: that's the primary api for gmusicapi, so I'm already familiar with it.

The only operation I found missing was changing playlist descriptions for existing playlists. I'll have to do a bit more research to see if the api supports this and the client just doesn't call it, or if the api genuinely doesn't support it.

@simon-weber
Copy link
Owner Author

Ok, I think I've verified that everything we need is supported 👍

It might be a bit until I have something people can test out. I'll try to keep this thread up to date along the way.

@simon-weber
Copy link
Owner Author

One hangup: since the last time I looked at this, Google seems to have locked down access to the api. I have a workaround, but it'll require either:

  1. keeping the existing client id and performing oauth manually for skyjam, which I'm not sure is possible
  2. switching to a different api project and performing oauth through chrome, which will force existing paying users to re-auth the extension

I'm leaning towards a new project since it makes the code much simpler and I think it's a relatively minor inconvenience. I've already got notification permission, so I can fire one to prompt the reauth.

A second issue is that I'm not yet sure the new api returns the last played date of tracks, which is a pretty important piece of metadata. They do have a field called "recentTimestamp", but if it's used in the same way as the webclient, it's not exactly what we want (since it also includes other metadata updates). I'll check into it.

Everything else is looking good so far!

@simon-weber simon-weber self-assigned this Nov 19, 2016
@simon-weber
Copy link
Owner Author

Yeah, recentTimestamp isn't what we want (I actually don't know what it's for. I haven't been able to get it to update). lastModifiedTimestamp does get updated on plays, but also on other metadata changes too.

I'm going to switch to looking into just the playlist operations. If that works out, we could potentially stick with the old api for retrieving tracks while using the new one for syncing.

@simon-weber
Copy link
Owner Author

simon-weber commented Nov 19, 2016

This will be a game changer for syncing. I managed to fully replace 5 max-length playlists in a single request (10k individual mutations), which was the original hope for #73.

Since it was a single call, it only triggered one ui refresh.

Also, since individual operations are low-cost, I can switch from a 3-part non-atomic remove/append/reorder (which reduces operations for playlists that mostly remain the same) to a much simpler 2-part atomic remove-all/append-all (which will always perform at most 2k * playlists operations). This means I don't need to handle reordering at all, which is a relief given that it's the flakiest part of the api.

Playlist updates can also be retrieved deferentially now (right now I have to poll the endpoints that retrieve all their contents).

I'm hoping to have something people can try out in a week or so. As of now, the plan is to cut a release with just the client id change + reauth prompt, then later on introduce a setting for people to opt in to the new syncing code. Longer term, I hope to switch everyone over to it and remove the old code entirely.

@simon-weber
Copy link
Owner Author

I'm also going to remove multi-user support, since getAuthToken can only be used on profile users anyway. While it's possible to keep it by switching to handling all oauth interaction manually, it's always been a bit half-baked and GA doesn't show anybody but me using it anyway.

@simon-weber
Copy link
Owner Author

3.1.0 is out with this behind a setting. Check out the announcement post for how to opt into it.

@simon-weber
Copy link
Owner Author

Unfortunately, this doesn't seem to be a syncing problem cure-all just yet, but the new api still gives me plenty of room to optimize.

The stats from Google's api console are promising: only 1% of requests are 5xxs. But, since Chrome blocks requests continually causing 5xxs that number undercounts the actual number of syncing failures.

GA has more representative stats: .1% of playlist syncs failed and 20% of entry syncs have failed. The average number of mutations in a successful entry sync is ~500, while failing syncs is 3k+, so the problems seems pretty clearly caused by mutation volume. The failing requests seem to time out at 10 seconds (presumably from the load balancer) consistently.

Splitting syncs into different batches by playlist may help but it's not ideal: it'd cause more ui refreshes. It also doesn't actually reduce the number of mutations, so I'm not sure it'd really help.

So, my plan is to switch away from the delete/add/reorder-all approach in favor of delete/add/reorder-min (which turns out to be an interesting application of longest increasing subsequence: the elements outside of it are the minimum number of spots to move). This should drastically reduce the number of reorder mutations in most cases. It's kind of messy given how the local and remote sides are represented, but I think I should have it finished in the next few days.

@simon-weber
Copy link
Owner Author

simon-weber commented Dec 4, 2016

I'm testing the reorder-min implementation now, and it seems to help. I've run into a few issues that I'm still working on:

  • reordering using entry ids (relativePositionType 1, following/preceding entry id as an actual entry id) doesn't seem to work reliably. The mutations are accepted and the order is changed, but the resulting order doesn't seem to make any sense. The case I was using to test this was flipping a playlist of length 3 by moving (0-indexed) 2 to 0 and 1 to 1. If I'm lucky, this is an edge case of how remaining values are treated after reorderings, and I can just switch my maxIncSub implementation to return the last element instead of the first.
  • since the api only allows one relativePositionType per mutation, it's impossible to properly order a batch of reordering and additions where they end up next to eachother (you'd need to make a reordering with one clientId and one entryId). I'm not really sure if there's much I can do about this; reordering may just have to be eventually consistent.

@simon-weber
Copy link
Owner Author

The first problem looks like a Google bug: performing the same operations on Android generates the same updates and the same incorrect result. I'll try to report it, but who knows if it'll ever get looked at.

Between these two issues I think we'll just have to settle for eventually consistent ordering for now.

@simon-weber
Copy link
Owner Author

Ok, I just pushed the min-reorder change in 3.2.2. It looks great in my testing: my library went from immediately causing 500s to syncing without trouble. Reorders seem to take 2 or 3 syncs to fully apply because of the problems described above, but in practice it doesn't seem like a big deal.

So long as the reporting shows this working well for everyone else, new syncing is pretty much ready to go. There's only one minor bug I've yet to fix (race conditions between manual/periodic sync, especially involving the playlist cache). I may even push that off until I've switched everyone over, since it'll be easier to fix after deleting the old syncing code.

If you were waiting to opt in to the new syncing, now's the time to do it!

@simon-weber simon-weber changed the title investigate switching to ios api switch to ios api Dec 4, 2016
@simon-weber
Copy link
Owner Author

Everything is looking good, so I've switched everyone over in 4.0.0.

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

No branches or pull requests

3 participants