square / retrofit

A type-safe HTTP client for Android and the JVM
https://square.github.io/retrofit/
Apache License 2.0
42.97k stars 7.3k forks source link

Allow access to OkHttp's WebSocket type #924

Open JakeWharton opened 9 years ago

JakeWharton commented 9 years ago
interface Service {
  @GET("/chat/{id}")
  Call<WebSocket> chat(
      @Path("id") String id);
}
swankjesse commented 9 years ago

Whoa.

JakeWharton commented 9 years ago

There's problems here:

WebSocket is solely a writing type. Messages that are read come in as callbacks to the WebSocketListener which is also what has the open/failure callbacks. Additionally, we are re-using the failure callback for both setup failure and then protocol or transport failure down the line.

A potential solution would be to split the listener into two callback classes: one for the connection and one for messages. This could be done either here as a set of wrappers, or in OkHttp. You may remember that my initial API for WebSocket was done in this way before you commented that we could consolidate them into one.

However, the split is not entirely a solution because we have a threading problems. When we get the WebSocket instance in onOpen we are on the reader thread. Once that method returns, it sits on the loop reading messages from the wire. If the call adapter choses to move threads before handing the WebSocket to the caller, there's a timing issue where messages might be dropped because a listener has not been set.

One potential solution would be to do the callback split, change onOpen to onConnected, and add a method like start(WebSocketListener) to WebSocket. This would just be a CountDownLatch that holds the reader thread up for a period of time (like the connect timeout that we already have on the OkHttpClient) in case threads need to be moved.

I don't have other solutions in mind yet so I'm open to those and thoughts on what I proposed above.

swankjesse commented 9 years ago

We'll want Retrofit's web socket to make onMessage take a <T>, and similarly for sendMessage.

I'm guessing Retrofit will encapsulate OkHttp's WebSocket and layer its own abstraction on top, to handle type adapters and threading. This is symmetric with Call, right?

JakeWharton commented 9 years ago

Making it typed is very interesting. And that will of course require an abstraction. I'll experiment with it.

JakeWharton commented 9 years ago

APIs to look at for inspiration/samples:

JakeWharton commented 9 years ago

Thinking about this more, I think it requires a WebSocketCall type in Retrofit which is parameterized on incoming type I and outgoing type O. Unfortunately this presents a problem for call adapters since they won't work with this type.

JakeWharton commented 9 years ago

Here's the untested prototype so far: https://github.com/square/retrofit/compare/jw/websocket

trustratch commented 8 years ago

I would like to see this happen, how can i help you guys

frxncisjoseph commented 8 years ago

Still waiting... :-(

frxncisjoseph commented 7 years ago

I'd at-least like some sort of update please? Is this going ahead or not?

jjhesk commented 7 years ago

how about now? any updates? its almost 2017

JakeWharton commented 7 years ago

Updates will be put as comments in this issue. No new comments means no new updates.

On Tue, Dec 13, 2016, 2:00 PM 世外桃源 notifications@github.com wrote:

how about now? any updates? its almost 2017

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/square/retrofit/issues/924#issuecomment-266829306, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEUNmMnc7iCD0D5P4ybfiQEbrl1GMks5rHutZgaJpZM4FNFSf .

taejungkim commented 7 years ago

I did not find anything about Web Sockets in Retrofit version 2.2. (https://github.com/square/retrofit/blob/parent-2.2.0/CHANGELOG.md) When will you add a web socket to Retrofit?

JakeWharton commented 7 years ago

There is no timeframe

On Sat, Mar 11, 2017, 10:53 PM taejungkim notifications@github.com wrote:

I did not find anything about Web Sockets in Retrofit version 2.2. When will you add a web socket to Retrofit?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/square/retrofit/issues/924#issuecomment-285920342, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEVIjZHMXuYOmUKhIcE2WlkZ9TNiDks5rk2xHgaJpZM4FNFSf .

taejungkim commented 7 years ago

I will use Okhttp version 3.5 websocket. However, I hope Retrofit will support web sockets.

MateuszNaKodach commented 7 years ago

When we should expect web sockets in retrofit ;) ?

forresthopkinsa commented 7 years ago

@nowakprojects For now, I think it's safe to assume that

There is no timeframe

EnnaKenT commented 6 years ago

Is there any news?

frxncisjoseph commented 6 years ago

@EnnaKenT Don't sell yourself dreams.

Albert221 commented 6 years ago

- knock knock - who's there? - developers waiting for Websockets in Retrofit - uhm, sorry, nobody's home now

JakeWharton commented 6 years ago

I'm available to hire to do this work, but if you want it for free then it's going to be on my schedule. Enjoy the rest of the free code in the mean time!

On Sat, Apr 7, 2018, 8:43 AM Albert notifications@github.com wrote:

  • knock knock
  • who's there?
  • developers waiting for Websockets in Retrofit
  • uhm, sorry, nobody's home now

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/square/retrofit/issues/924#issuecomment-379466703, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEVx_Sojk3eIBYdC2ZLTwsatkyDTEks5tmLRkgaJpZM4FNFSf .

swankjesse commented 6 years ago

Umm yes I’ll pay when can you start.

forresthopkinsa commented 6 years ago

All in support of locking this thread until it becomes relevant?

JakeWharton commented 6 years ago

I don't think that's necessary. With any luck we'll be able to knock this out in the next few months.

On Mon, Apr 9, 2018, 11:08 AM Forrest Hopkins notifications@github.com wrote:

All in support of locking this thread until it becomes relevant?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/square/retrofit/issues/924#issuecomment-379842592, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEaXNJzda1V1iVMWB-sW8e2WrR-OGks5tm6OWgaJpZM4FNFSf .

mhdtouban commented 6 years ago

For those still waiting for WebSocket support.. tinder came up with Scarlet a Retrofit for WebSockets. https://github.com/Tinder/Scarlet

forresthopkinsa commented 6 years ago

@mhdtouban and it's in Kotlin, too! Gorgeous.