davidstump / SwiftPhoenixClient

Connect your Phoenix and iOS applications through WebSockets!
MIT License
506 stars 146 forks source link

Question: How do I know a socket was successfully opened? `isConnected` seems to say whether or not the handshake took place, not whether the socket was actually opened. #225

Closed msandel6 closed 1 year ago

msandel6 commented 1 year ago

Initially I had assumed that socket.isConnected would tell me whether or not a socket is open.

However, that value is set to .open in the URLSession:webSocketTask:didOpenWithProtocol: delegate function, which according to Apple indicates "that the WebSocket task successfully negotiated the handshake with the endpoint, indicating the negotiated protocol".

Is there something I'm missing? Where can I retrieve info about whether the connection was successfully upgraded to a socket post-handshake?

Edit: For additional context - I'm working with a situation in which the server may refuse the socket connection based on parameters sent. From what I understand, the library seems to assume that the connection will always be successful if the handshake/HTTP request was.

dsrees commented 1 year ago

Are you looking for socket.onOpen()?

msandel6 commented 1 year ago

.onOpen() is called from within that same URLSession delegate method, so unless I'm missing something that would be called under the same circumstances. 🤔

dsrees commented 1 year ago

I'm sorry, i'm not 100% following what the problem exactly is that you're trying to solve for. Is there a different method that URLSession delegate provides that, if used, solves your usecase?

msandel6 commented 1 year ago

The problem is that the didOpenWithProtocol delegate method gets called after a successful TCP handshake, rather than after the socket is actually opened.

In my case, this is the sequence of events:

  1. I send the open request with parameters
  2. The server receives it (after successful handshake)
  3. The didOpenWithProtocol delegate callback happens due to the successful handshake
  4. The SwiftPhoenixClient framework sets socket.isConnected to true and calls socket.onOpen(). My app responds accordingly, entering a flow that assumes the socket is open.
  5. The server rejects the request to upgrade the connection to a socket if the parameters/credentials I sent are incorrect

The app now thinks there is an open socket connection, even though the socket was never opened.

Unfortunately, it doesn't look like there are other URLSessionWebSocketDelegate methods we can use to determine when a socket has actually been opened. Maybe there is a possible workaround using pings where at least one pong has to be returned before the socket is declared as isConnected, but I haven't had time to experiment with that.

dsrees commented 1 year ago

Okay, i'm following now. Thank you for clarifying. I can't speak directly to the solution, but I would assume your server can allow the upgrade to the websocket but then proceed to close the connection with an error? I don't think there is anything client side to intercept a failed upgraded handshake

nathanl commented 1 year ago

@dsrees From a Phoenix perspective, preventing the upgrade for failed authentication is a supported feature:

To deny connection, return :error or {:error, term}. To control the response the client receives in that case, define an error handler in the websocket configuration. https://hexdocs.pm/phoenix/1.7.0-rc.2/Phoenix.Socket.html#c:connect/2-socket-params-and-assigns

Looks like we can do it at the channel layer too, but I think the normal use case there is like "you're allowed to connect to the chat server (Socket) but you're not allowed to join this specific room (Channel)." For dealing with an authentication error ("we don't know who you are"), the Socket seems like a more correct place to deny the user.

We can work around this, I just wanted to give that perspective.

nathanl commented 1 year ago

I don't think there is anything client side to intercept a failed upgraded handshake

This seems like a bug, doesn't it? If you don't get a 101 Swiching Protcols response but assume that it worked, does that mean you wouldn't get an error if you tried to establish a connection to a REST endpoint?

dsrees commented 1 year ago

@nathanl @msandel6 thanks for some additional context. I believe URLSession handles the 101 Switching Protocols response internally and doesn't seem to expose that level of detail to the delegate.

Testing this against a local server, if I return :error from Socket.connect/3, then the client receives the error and attempts to reconnect, which is the same behavior I see the Phoenix JS client performing. This error will be passed to your app via socket.onError { error in ... } callback where you can inspect it further if you'd like. Inside of this socket.onError callback, socket.isConnected is returning false for me and URLSession:webSocketTask:didOpenWithProtocol: is never being called.

That all being said, I am not able to reproduce the same behavior described earlier:

  1. The didOpenWithProtocol delegate callback happens due to the successful handshake
  2. The SwiftPhoenixClient framework sets socket.isConnected to true and calls socket.onOpen(). My app responds accordingly, entering a flow that assumes the socket is open.

If you can create a small client/server repo that reproduces this error, then I'd be happy to look further into this. For reference, I am testing using the ChatRoomViewController found in the client's example project and this chat server locally.

msandel6 commented 1 year ago

@dsrees Thanks for the response! So when you return an error it never even enters the .onOpen delegate functions? In my case the client error delegate does get hit (and .isConnected set to false), but only after first going through .onOpen and setting .isConnected to true. So I still can't rely on the .isConnected value unless I add an arbitrary wait time to ensure it isn't closely followed by an error.

dsrees commented 1 year ago

@msandel6 that is correct. If you can reproduce it in a toy ios/phoenix project then I am happy to pull that and take a look.

dsrees commented 1 year ago

@msandel6 @nathanl Looking into this a little deeper and I found an issue with how the Client is passing errors back through onError. Reading the docs revealed this about didCompleteWithError

The only errors your delegate receives through the error parameter are client-side errors, such as being unable to resolve the hostname or connect to the host. To check for server-side errors, inspect the response property of the task parameter received by this callback.

In order to get the 401 from the server response, the client needs to return back the task.response to the calling code as well which should inform you of any potential server-side errors. I'm going to work on getting this added today. Hopefully that will help with whatever solution you guys came to.

msandel6 commented 1 year ago

Interesting. Makes sense to me, thanks so much for investigating @dsrees!

dsrees commented 1 year ago

5.2.0 has been released with this fix. You can see it in action here