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 autoreconnect #65

Merged
merged 5 commits into from
Jun 29, 2016
Merged

Fix autoreconnect #65

merged 5 commits into from
Jun 29, 2016

Conversation

dral3x
Copy link
Contributor

@dral3x dral3x commented Jun 29, 2016

After some debugging, I fixed the autoreconnect feature.
Now, the retry happens only if
1- the flag is set to true in the options
2- the socket disconnection occurred because of an error

Hope you merge this asap in master. Thanks

@hamchapman
Copy link
Contributor

I'm not sure how I feel about the changes in the Starscream file. I'd like to be able to keep tracking the Starscream library without any custom code if possible.

Are those changes required?

@dral3x
Copy link
Contributor Author

dral3x commented Jun 29, 2016

Oh ok. Well, the problem is that Starscream generate an NSError on disconnect even if the connection is closed with Normal code. To me this is not correct but if we want to leave that library as it is, we need to filter out that error when it arrives.

@hamchapman
Copy link
Contributor

Yeah, I've just tried this myself and it does seem to be the case.

I think we can check if the error's localizedDescription is "connection closed by server" and the error code is 1000 (normal close code).

If that's the case then don't try and reconnect. How does that sound to you?

@dral3x
Copy link
Contributor Author

dral3x commented Jun 29, 2016

Done.
I will not check for the error message (makes no sense). I checked for the code value.

eventHandler.callback(eventData)
}

let jsonize = connection.options.attemptToReturnJSONObject ?? false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add the ?? false ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry it's because it's not based on #60

@hamchapman hamchapman merged commit 0a378df into pusher:master Jun 29, 2016
@dral3x
Copy link
Contributor Author

dral3x commented Jun 29, 2016

Thanks man :)

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