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 race condition on authorization #77

Closed
wants to merge 2 commits into from

Conversation

xAockd
Copy link

@xAockd xAockd commented Feb 4, 2019

#76

@FZambia
Copy link
Member

FZambia commented Feb 4, 2019

Looks like you forgot to include part of your fix into this pr - _subscribeXHR is always null here. Anyway I'll try to extend this to generic case, thanks!

@xAockd
Copy link
Author

xAockd commented Feb 4, 2019

Omg, yes. I'll add it early in the morning

@FZambia
Copy link
Member

FZambia commented Feb 4, 2019

@xAockd could you please check #78 instead - I tried to extrapolate your solution to other cases. Also additionally check client ID to be actual.

@xAockd
Copy link
Author

xAockd commented Feb 5, 2019

@FZambia I've checked your PR. Really good job, very fast reaction on the issue! It should solve the issue and it covers all requests. But, I think that we have to improve the code and follow DRY principle.
If it's okay to you, can we merge it and think about improvenments later?
Thanks

@FZambia
Copy link
Member

FZambia commented Feb 5, 2019

Thanks for looking. Yep, agree with you about dry - let's merge it anyway, will appreciate pr fixing code repeatition. I am a bit busy at work these days and work on Swift client and investigating possible Java client for Centrifugo at spare time at moment so any help is much appreciated

@xAockd xAockd closed this Feb 5, 2019
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.

2 participants