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

Replace starscream with iOS 13+ URLSessionWebSocketTask #20

Merged
merged 2 commits into from
May 17, 2024

Conversation

stuartcamerondeakin
Copy link
Contributor

@stuartcamerondeakin stuartcamerondeakin commented Apr 11, 2024

The starscream library doesn't correctly report an initial connection failure to the SwiftStomp when using the default TCPTransport.swift transport library, and despite there being two pending PRs daltoniam/Starscream#904 and daltoniam/Starscream#821 which both attempt to address the issue neither has been merged for 3 and 4 years each respectively. Hopes of this being resolved any time soon are not high.

This makes use of SwiftStomp in a production environment where connectivity needs to be ensured with adequate retries etc impossible.

The alternative if running on iOS 13+ is to use the NativeTransport.swift library internally, but that's just a very thin wrapper for Apple's URLSessionWebSocketTask, making the use of Starscream itself fairly redundant.

I've therefore removed the dependency on Starscream and have implemented the underlying WebSocket transport natively using URLSessionWebSocketTask. Surprise 🎉!

This is a big PR but makes the library much more robust, simplifies some things and removes the dependency on Starscream which seems to have always been problematic at best.

Cases that have been tested:

  • Initial connection failure to the websocket with automatic reconnection enabled works
  • Connection drops on an existing connection also result in automatic reconnection attempts
  • Messages are being sent to topics correctly and subscription continues to work as-is
  • SPM dependency works, cocopods dependencies are not tested as we don't use that

@Romixery Romixery merged commit af5d7ad into Romixery:master May 17, 2024
@Romixery
Copy link
Owner

I appreciate it for this great update. @stuartcamerondeakin I've updated your PR by adding extra functionalities and improvements and merged it in master.
Thank you for your collaboration.

@stuartcamerondeakin
Copy link
Contributor Author

Thanks for building such a useful library in the first place! We really needed something robust for our use-case, and your library was tried & tested, but was just being let down by the underlying library. I took a gamble making the PR, but glad you approved of what was a pretty drastic change and I hope others get value from it too. Thanks again.

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