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

Fix refreshing token #255

Closed

Conversation

marioortizmanero
Copy link
Collaborator

Description

This fixes #224 by creating a wrapper for Mutex so that it's the same for both sync and async programming, similarly to how pagination works.

marioortizmanero and others added 13 commits September 15, 2021 15:08
Over in spotify-tui, I implemented the new `start_context_playback` and hit "request unauthorized" every time.

Looking at the code, the previous `put` call was not receiving the auth headers. This fix here is to use the `endpoint_put` method which seems to abstract passing in the headers?
Fix oauth set token and fix unauthorized playback control
@marioortizmanero marioortizmanero changed the base branch from master to ramsay_support_refresh_token September 25, 2021 11:59
@marioortizmanero
Copy link
Collaborator Author

Ok so I've got it compiling without needing lots of #[cfg()]. The only problem is that there are probably lots of deadlocks, I should try to fix these before this is merged.

@marioortizmanero
Copy link
Collaborator Author

marioortizmanero commented Sep 25, 2021

What do you think about renaming get_token to refresh_and_lock_token, @ramsayleung? Maybe it's too long but that function does a lot of things so it may make sense. I can do that in another PR to avoid making this one a mess.

@marioortizmanero
Copy link
Collaborator Author

While I'm at it I've also merged master onto this branch and fixed all the conflicts

@marioortizmanero
Copy link
Collaborator Author

marioortizmanero commented Sep 25, 2021

Damnit, I've completely messed up the diff by merging master...

You can take a look at the commits manually:

@marioortizmanero
Copy link
Collaborator Author

Well, this has become a mess. I'm having lots of trouble merging master... Would you mind re-doing the PR with the current master branch, @ramsayleung? It's probably going to be much easier than trying to merge with this super outdated branch.

It shouldn't be too hard, you can copy-paste most stuff (the examples and the mutex module, for one)

@ramsayleung
Copy link
Owner

ramsayleung commented Oct 9, 2021

Since everything else has been merged, we could continue work on #224 :)

Would you mind re-doing the PR with the current master branch, @ramsayleung? It's probably going to be much easier than trying to merge with this super outdated branch.

Do you mean I should create a new PR from the current master branch?

@marioortizmanero
Copy link
Collaborator Author

Do you mean I should create a new PR from the current master branch?

Yes. Unfortunately the merge conflicts are super hard to deal with, it'll be easier to do it from scratch in my opinion. Then you can add what I suggested at #255 to fix our problem.

@ramsayleung
Copy link
Owner

ramsayleung commented Oct 10, 2021

Did you run the with_auto_reauth example successfully? I got an stackoverflow error when I ran it. And I have created a new PR #262

@marioortizmanero
Copy link
Collaborator Author

That most likely means that there's unbounded recursivity somewhere, i. e. the auto reauth function calls something which calls the auto reauth again. I'll take a look later.

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.

None yet

4 participants