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

Resource management #6300

Open
6 tasks
martinbonnin opened this issue Dec 3, 2024 · 0 comments
Open
6 tasks

Resource management #6300

martinbonnin opened this issue Dec 3, 2024 · 0 comments

Comments

@martinbonnin
Copy link
Contributor

martinbonnin commented Dec 3, 2024

Description

This is a tracking issue for improving resource management in Apollo Kotlin. There's a tradeoff between:

  • eagerly release all resources
  • sharing OkHttp connection pool and dispatcher (kdoc)

"Soft" resources (will be eventually released but close() is required for prompt release):

  • BatchingHttpInterceptor.scope
  • WebSocketNetworkTransport.coroutineScope (ws)
  • WebSocketPool (websocket)
  • OkHttpClient.dispatcher.executorService
  • OkHttpClient.conectionPool
  • writeToCacheAsynchronously

"Hard" resources (require a call to close() or will leak forever or display a warning)

  • AndroidSqliteDriver.close()
  • NSUrlSession

Issues with closing resources:

Note: OkHttp uses daemon threads since 4.5.0+ and hanging apps shouldn't be an issue anymore

TODO:

  • Move writeToCacheAsynchronously to the caller coroutine scope. This will remove a resource to manage.
  • Deprecate (and ultimately remove) ApolloClient.close(). Since we want to encourage sharing of the network resource
  • Rename NetworkTransport.dispose() to NetworkTransport.close()
  • Add ApolloClient.Builder.normalizedCache(ApolloStore) and promote it as the main entry point for the normalized cache. By having the caller create the ApolloStore, it makes it more obvious that it needs to be closed.
  • Decide what to do with HttpInterceptor.close()
  • Update all tests to release all their resources always.

At the end of the day, I think a good target to close resources is:

apolloClient.networkTransport.close()
apolloClient.subscriptionNetworkTransport.close()
apolloClient.apolloStore.close()

NetworkTransport and ApolloStore would take ownership of their resources (contrary to ApolloClient). This is because they are interfaces. A NetworkTransport could possibly be closed with networkTransport.okHttpClient.connectionPool.evictAll() but another would need something else so we need the NetworkTransport.close().

Doing the above invalidate other clients sharing the NetworkTransport/ApolloStore (for an example, created through newBuilder). We might want to add special wording to newBuilder() to guard against sharing an ApolloStore (which, although shareable is potentially dangerous)

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

No branches or pull requests

1 participant