Introduce incubating WebSocketEngine #5676
Conversation
✅ Deploy Preview for apollo-android-docs canceled.
|
| /** | ||
| * Creates a new [WebSocket]. | ||
| * | ||
| * The [WebSocket] is be garbage collected when not used anymore. Call [WebSocket.close] to close the websocket and release resources earlier. |
There was a problem hiding this comment.
| * The [WebSocket] is be garbage collected when not used anymore. Call [WebSocket.close] to close the websocket and release resources earlier. | |
| * The [WebSocket] will be garbage collected when not used anymore. Call [WebSocket.close] to close the websocket and release resources earlier. |
Actually maybe the 1st sentence can be removed? As it's true of any type.
There was a problem hiding this comment.
I'm on a crusade to remove future tense from our docs. It's what our style guide says (internal link) and I tend to agree despite being guilty of a lot of the current usage.
I'll remove the extra 'be' but I'm in favor of keeping the sentence. IO streams like InputStream usually need closing because they hold file descriptors or other resources. Here the file descriptor is released automatically on websocket error or server close. It might become the norm at some point but I don't think we're there yet.
| val d = JSON.stringify(data2) | ||
| if (!disposed) { | ||
| disposed = true | ||
| listener.onError(DefaultApolloException("The JS WebSocket implementation only support text messages (got $d)")) |
There was a problem hiding this comment.
Isn't the Buffer.isBuffer(data2) case for data messages?
There was a problem hiding this comment.
👍 I'll update the message
|
|
||
| import okhttp3.OkHttpClient | ||
|
|
||
| internal val defaultOkHttpClientBuilder: OkHttpClient.Builder by lazy { |
There was a problem hiding this comment.
Why not just calling OkHttpClient.Builder() on demand?
There was a problem hiding this comment.
I want to share the OkHttp threadpool between WebSockets and HTTP. So this is technically not used right now but I'll enable it when swapping the websocket network transport is not incubating anymore.
The incubating WebSocketEngine uses events instead of coroutines, making it easier to map to the underlying implementations (OkHttp, NSUrlSession and JS WebSocket are all event-based).
It also fixes a memory leak in the Apple WebSocketEngine (the NSUrlSession wasn't cancelled).
This PR also makes wasm tests "work" again. They are mostly ignored but you can now type
./gradlew buildand the build will pass. They were failing previously because trying to run on a node version that did not support wasm.