Basic NSURLSessionWebSocketTask functionality #1541

maratal · 2022-11-28T12:42:36Z

…sionWebSocketTask # Conflicts: # Source/ARTWebSocketTransport.m

maratal · 2022-12-12T13:34:05Z

Because this will replace core/critical functionality, it is probably a good idea to introduce it gradually via something like clientOptions.useNativeWebSocket and explicitly ask some clients to give it a try. WDYT @lawrence-forooghian @QuintinWillison

lawrence-forooghian · 2022-12-12T14:22:43Z

I'll probably need to take some time to get some understanding of WebSockets in general (perhaps reading the RFC) before I can give this a proper review. I'll see if I can find some time this week for that, but can't guarantee that I'll be able to.

lawrence-forooghian · 2022-12-12T14:42:51Z

Because this will replace core/critical functionality, it is probably a good idea to introduce it gradually via something like clientOptions.useNativeWebSocket and explicitly ask some clients to give it a try. WDYT @lawrence-forooghian @QuintinWillison

It's definitely worth thinking about ways to get confidence in this change, yeah.

QuintinWillison · 2022-12-13T08:43:55Z

I don't think that adding a client option to change core runtime behaviour is going to do anything to explain this to time poor customers, nor drive them to use it. My preference would be to use the tools already available to us to make this change happen (versioning). So, perhaps, we should be discussing this as part of scope for version 2 of this SDK? (i.e. a major version bump) -- this should count as an enhancement from a customer's perspective, as well as from ours as maintainers. FYI, @mikelee638

lawrence-forooghian

It's exciting to think that we might be able to get rid of an external dependency at some point, so I'm happy to see this PR! I've popped some initial thoughts on here. I haven't looked at the implementation of ARTURLSessionWebSocket yet, since I'd like to understand the ARTWebSocket* protocols better first (see my comment re documentation).

Also, did you give any thought to the fact that, as it stands, CI will no longer be testing that the SDK works correctly when it uses SocketRocket, since we only test on:

iOS 16.0

tvOS 16.1

The version of macOS specified by GitHub Actions’ macos-latest runner image.

Perhaps we need to add some older OS versions there too?

I also think that making the change that's proposed here makes sense mainly if we’re intending to drop support for iOS < 13 at some point. If not, then we're just going to end up having to maintain two WebSocket implementations instead of one. Do you have thoughts on this @QuintinWillison or @mikelee638?

lawrence-forooghian · 2022-12-19T19:33:12Z

Ably.xcodeproj/project.pbxproj

@@ -3414,6 +3430,7 @@
 				SDKROOT = macosx;
 				SWIFT_ACTIVE_COMPILATION_CONDITIONS = DEBUG;
 				SWIFT_OBJC_BRIDGING_HEADER = "Spec/AblySpec-Bridging-Header.h";
+				SWIFT_OPTIMIZATION_LEVEL = "-Onone";


Is this related to the contents of the PR?

4793ebd
(Sorry this thing pops up every time, I should create an issue for it already 🤌)

lawrence-forooghian · 2022-12-19T19:39:32Z

Source/ARTURLSessionWebSocket.h

+NS_ASSUME_NONNULL_BEGIN
+
+API_AVAILABLE(macos(10.15), ios(13.0), watchos(6.0), tvos(13.0))
+@interface ARTURLSessionWebSocket : NSObject <ARTWebSocket, NSURLSessionWebSocketDelegate>


The NSURLSessionWebSocketDelegate conformance seems like an implementation detail; can we keep it private by placing its declaration in the .m file?

lawrence-forooghian · 2022-12-19T19:43:28Z

Source/ARTWebSocket.h

@@ -5,16 +5,24 @@

 NS_ASSUME_NONNULL_BEGIN

+@class ARTLog;


Is this needed?

lawrence-forooghian · 2022-12-19T19:46:27Z

Source/ARTWebSocket.h

+    ARTWS_CLOSING      = 2,
+    ARTWS_CLOSED       = 3,
+};
+
 /**
 This protocol has the subset of ARTSRWebSocket we actually use.
 */
 @protocol ARTWebSocket <NSObject>


Part of my task in reviewing ARTURLSessionWebSocket is to confirm whether it seems to correctly conform to the ARTWebSocket protocol, which I can't currently do since this protocol has no documentation. I think that now would be a good moment to add documentation to this protocol. What do you think?

(Same goes for ARTWebSocketDelegate; to review ARTURLSessionWebSocket I need to verify whether it respects the contract of the delegate protocol, which I can't do since it's not documented.)

lawrence-forooghian · 2022-12-19T19:50:34Z

Source/ARTWebSocketTransport.m

@@ -319,6 +325,8 @@ - (ARTRealtimeTransportError *)classifyError:(NSError *)error {
        type = ARTRealtimeTransportErrorTypeHostUnreachable;
    } else if ([error.domain isEqualToString:@"NSPOSIXErrorDomain"] && (error.code == 57 || error.code == 50)) {
        type = ARTRealtimeTransportErrorTypeNoInternet;
+    } else if ([error.domain isEqualToString:@"NSURLErrorDomain"] && (error.code == -1009)) {


Suggested change

} else if ([error.domain isEqualToString:@"NSURLErrorDomain"] && (error.code == -1009)) {

} else if ([error.domain isEqualToString:NSURLErrorDomain] && (error.code == NSURLErrorNotConnectedToInternet)) {

Also, it feels to me like the error classification should probably be delegated to the implementations of the ARTWebSocket protocol. If the point of using a protocol is to hide the implementation details of the WebSocket class (i.e. whether we're using SocketRocket or NSURLSession), then it doesn't make sense to be checking for SocketRocket-specific error codes here, for example. Furthermore, if in the future we want to remove SocketRocket, it's going to be harder to do because we won't know which lines in this method are safe to remove because they only applied to SocketRocket.

lawrence-forooghian · 2022-12-19T19:55:35Z

Source/ARTWebSocket.h

 @protocol ARTWebSocketDelegate;

+typedef NS_ENUM(NSInteger, ARTWebSocketState) {
+    ARTWS_CONNECTING   = 0,


Style: I think that ARTWebSocketStateConnecting etc would be more appropriate.

lawrence-forooghian · 2022-12-19T19:57:55Z

Source/ARTURLSessionWebSocket.m

+
+@end
+
+@implementation ARTURLSessionWebSocket {


Given how fundamental this class is to the SDK, I think it's important that we unit test it, to confirm that it conforms to the ARTWebSocket protocol and that it respects the contract of the ARTWebSocketDelegate protocol.

lawrence-forooghian · 2022-12-19T20:08:20Z

@QuintinWillison:

So, perhaps, we should be discussing this as part of scope for version 2 of this SDK? (i.e. a major version bump) -- this should count as an enhancement from a customer's perspective, as well as from ours as maintainers. FYI, @mikelee638

I'm not sure I understand how versioning would help us here – this change is meant to be a backwards-compatible enhancement so it wouldn't warrant a major version bump. If we did want to use versioning to provide customers with a way to opt in to this new code (which, as you say, I'm not sure many would be interested in doing), then it would probably make more sense to use a pre-release version.

…legate`.

…ARTWebSocket` protocol.

QuintinWillison · 2023-01-03T09:50:29Z

Thanks for your comments, @lawrence-forooghian. You're right regarding versioning, though perhaps it might be a version 2 scope change if we're going to significantly increment minimum OS runtime version(s). 🤔 You also commented:

I also think that making the change that's proposed here makes sense mainly if we’re intending to drop support for iOS < 13 at some point. If not, then we're just going to end up having to maintain two WebSocket implementations instead of one.

Maintaining two WebSocket implementations is not ideal, both in terms of maintainability of the codebase and how it is tested. So my preference would be to drop support for the older iOS runtimes as soon as we can.

lawrence-forooghian · 2024-04-15T13:09:44Z

Taking another look at this, in the context of @umair-ably’s RFC for dropping support for iOS < 14. @maratal, do you remember whether your work here concluded that NSURLSessionWebSocketTask would be an adequate replacement for SocketRocket? Also, I noticed that the Network framework provides WebSocket functionality through NWProtocolWebSocket, also introduced in iOS 13; do you know if/how this differs to NSURLSessionWebSocketTask?

maratal · 2024-04-15T13:35:41Z

I noticed that the Network framework provides WebSocket functionality through NWProtocolWebSocket, also introduced in iOS 13; do you know if/how this differs to NSURLSessionWebSocketTask?

Looks like it's swift only. And NSURLSessionWebSocketTask is an objective-c wrapper around it. That's my conclusion from the first glance.

lawrence-forooghian · 2024-04-15T13:46:51Z

Ah, OK. There is also a C version, though. Apple’s guidance is to favour the Network framework:

Unless you have a specific reason to use URLSession, use Network framework for new WebSocket code.

maratal · 2024-04-15T14:23:16Z

Ah, OK. There is also a C version, though. Apple’s guidance is to favour the Network framework:

Unless you have a specific reason to use URLSession, use Network framework for new WebSocket code.

That's interesting, will check in depth the effort needed to move to c-version, thanks @lawrence-forooghian

lawrence-forooghian · 2024-04-15T14:26:37Z

If we drop support for iOS < 14, as is being proposed, we can then start using Swift inside the codebase anyway

Basic NSURLSessionWebSocketTask functionality

5e915b4

maratal marked this pull request as draft November 28, 2022 12:43

github-actions bot temporarily deployed to staging/pull/1541/jazzydoc November 28, 2022 12:49 Inactive

Check readyState before setting.

0319753

github-actions bot temporarily deployed to staging/pull/1541/jazzydoc December 4, 2022 22:55 Inactive

Weakified self + added logger

30d0fc1

github-actions bot temporarily deployed to staging/pull/1541/jazzydoc December 5, 2022 17:34 Inactive

maratal added 2 commits December 10, 2022 20:33

Merge remote-tracking branch 'origin/main' into feature/1540-NSURLSes…

95606a5

…sionWebSocketTask # Conflicts: # Source/ARTWebSocketTransport.m

Removed unnecessary null specifiers

2d685ef

github-actions bot temporarily deployed to staging/pull/1541/jazzydoc December 10, 2022 22:43 Inactive

maratal marked this pull request as ready for review December 10, 2022 23:13

Fixed reconnection issue

0a13cd6

maratal force-pushed the feature/1540-NSURLSessionWebSocketTask branch from a3e6134 to 0a13cd6 Compare December 11, 2022 21:28

github-actions bot temporarily deployed to staging/pull/1541/jazzydoc December 11, 2022 21:38 Inactive

maratal requested review from lawrence-forooghian and QuintinWillison December 11, 2022 22:02

lawrence-forooghian reviewed Dec 19, 2022

View reviewed changes

maratal added 8 commits December 24, 2022 22:26

Removed unintended setting change

4793ebd

Removed unnecessary declaration

565a550

Moved delegate conformance declaration to implementation file

b7b9be9

Scheduling on main dispatch queue if delegateDispatchQueue is not set.

b05377d

Removed unnecessary declaration.

297a0b9

Cleaned nullable specifiers

78efcd9

Added documentation commentary for ARTWebSocket and `ARTWebSocketDe…

7374511

…legate`.

Renamed enum constants.

7aef468

Replaced numeric error code with constant.

2752fab

github-actions bot temporarily deployed to staging/pull/1541/jazzydoc December 25, 2022 21:36 Inactive

Error classification is now delegated to the implementations of the `…

703482c

…ARTWebSocket` protocol.

github-actions bot deployed to staging/pull/1541/jazzydoc December 25, 2022 23:47 View deployment

QuintinWillison removed their request for review February 28, 2023 11:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic NSURLSessionWebSocketTask functionality #1541

Basic NSURLSessionWebSocketTask functionality #1541

maratal commented Nov 28, 2022

maratal commented Dec 12, 2022

lawrence-forooghian commented Dec 12, 2022

lawrence-forooghian commented Dec 12, 2022

QuintinWillison commented Dec 13, 2022

lawrence-forooghian left a comment

lawrence-forooghian Dec 19, 2022

maratal Dec 25, 2022 •

edited

Loading

lawrence-forooghian Dec 19, 2022

maratal Dec 25, 2022

lawrence-forooghian Dec 19, 2022

maratal Dec 25, 2022

lawrence-forooghian Dec 19, 2022

maratal Dec 25, 2022

lawrence-forooghian Dec 19, 2022

lawrence-forooghian Dec 19, 2022

maratal Dec 25, 2022 •

edited

Loading

lawrence-forooghian Dec 19, 2022

maratal Dec 25, 2022

lawrence-forooghian Dec 19, 2022

lawrence-forooghian commented Dec 19, 2022

QuintinWillison commented Jan 3, 2023

lawrence-forooghian commented Apr 15, 2024 •

edited

Loading

maratal commented Apr 15, 2024

lawrence-forooghian commented Apr 15, 2024

maratal commented Apr 15, 2024

lawrence-forooghian commented Apr 15, 2024

	} else if ([error.domain isEqualToString:@"NSURLErrorDomain"] && (error.code == -1009)) {
	} else if ([error.domain isEqualToString:NSURLErrorDomain] && (error.code == NSURLErrorNotConnectedToInternet)) {


		@end

		@implementation ARTURLSessionWebSocket {

Basic NSURLSessionWebSocketTask functionality #1541

Are you sure you want to change the base?

Basic NSURLSessionWebSocketTask functionality #1541

Conversation

maratal commented Nov 28, 2022

maratal commented Dec 12, 2022

lawrence-forooghian commented Dec 12, 2022

lawrence-forooghian commented Dec 12, 2022

QuintinWillison commented Dec 13, 2022

lawrence-forooghian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maratal Dec 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maratal Dec 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawrence-forooghian commented Dec 19, 2022

QuintinWillison commented Jan 3, 2023

lawrence-forooghian commented Apr 15, 2024 • edited Loading

maratal commented Apr 15, 2024

lawrence-forooghian commented Apr 15, 2024

maratal commented Apr 15, 2024

lawrence-forooghian commented Apr 15, 2024

maratal Dec 25, 2022 •

edited

Loading

maratal Dec 25, 2022 •

edited

Loading

lawrence-forooghian commented Apr 15, 2024 •

edited

Loading