coil-kt / coil

Image loading for Android and Compose Multiplatform.
https://coil-kt.github.io/coil/
Apache License 2.0
10.66k stars 649 forks source link

Strict mode: Untagged network socket #46

Closed saschpe closed 5 years ago

saschpe commented 5 years ago

Describe the bug The underlying network socket managed by OkHttp isn't tagged according to policy. Strict mode complains about it. While it's a longstanding OkHttp issue other libraries apply a fixed tag.

To Reproduce Enable strict mode in an app's Application class during onCreate():

if (BuildConfig.DEBUG) {
    StrictMode.enableDefaults()
}

Expected behavior A clear and concise description of what you expected to happen.

Logs/Screenshots

2019-08-19 16:00:08.151 17277-17363/foo D/StrictMode: StrictMode policy violation: android.os.strictmode.UntaggedSocketViolation: Untagged socket detected; use TrafficStats.setThreadSocketTag() to track all network usage
        at android.os.StrictMode.onUntaggedSocket(StrictMode.java:2023)
        at com.android.server.NetworkManagementSocketTagger.tag(NetworkManagementSocketTagger.java:82)
...
        at okhttp3.internal.connection.RealConnection.connectSocket(RealConnection.java:245)
...

Library version 0.6.1

colinrtwhite commented 5 years ago

Thanks for the report. Do you have an example of other libraries working around this issue? I'm a little hesitant to work around this issue in the base Coil package. You can set a custom OkHttpClient which applies the work around like so:

Coil.setDefaultImageLoader {
    ImageLoader(this) {
         okHttpClient {
             // Apply the socket factory work-around.
         }
    }
}
benjamin-bader commented 5 years ago

If you decide not to add a tag in the base package, it might be nice to log a warning instead if the default, untagged, client is in use and strict-mode is on. If nothing else, it will save library users from googling the error and blaming Coil.

colinrtwhite commented 5 years ago

@benjamin-bader Strict mode will already log a warning, which references OkHttp and not Coil.

benjamin-bader commented 5 years ago

This is true, but will lead people to think that coil is the problem. A helpful message would perhaps avert a number of spurious github issues, but if you think the existing message form strict mode is fine, then that's all that matters.

colinrtwhite commented 5 years ago

Unfortunately, I don't think there's a way to show a message for this issue without requiring the user to install a custom socket factory. I'm going to close this issue for the moment + post the work-around as applied to Coil. If we get duplicate Github issues, we can point users back to this thread.

Here's the suggested work-around applied to Coil:

class TaggedSocketFactory(private val delegate: SocketFactory = getDefault()) : SocketFactory() {

    override fun createSocket(): Socket {
        val socket = delegate.createSocket()
        return configureSocket(socket)
    }

    override fun createSocket(host: String, port: Int): Socket {
        val socket = delegate.createSocket(host, port)
        return configureSocket(socket)
    }

    override fun createSocket(
        host: String,
        port: Int,
        localAddress: InetAddress,
        localPort: Int
    ): Socket {
        val socket = delegate.createSocket(host, port, localAddress, localPort)
        return configureSocket(socket)
    }

    override fun createSocket(host: InetAddress, port: Int): Socket {
        val socket = delegate.createSocket(host, port)
        return configureSocket(socket)
    }

    override fun createSocket(
        host: InetAddress,
        port: Int,
        localAddress: InetAddress,
        localPort: Int
    ): Socket {
        val socket = delegate.createSocket(host, port, localAddress, localPort)
        return configureSocket(socket)
    }

    private fun configureSocket(socket: Socket): Socket {
        TrafficStats.tagSocket(socket)
        return socket
    }
}

val imageLoader = ImageLoader(context) {
    val client = OkHttpClient.Builder().socketFactory(TaggedSocketFactory()).build()
    okHttpClient(client)
}

// If you use the Coil singleton...
Coil.setDefaultImageLoader(imageLoader)
Kolyall commented 2 years ago

See https://stackoverflow.com/a/36840978/2425851 By this answer we need also before TrafficStats.tagSocket(socket) call TrafficStats.setThreadStatsTag . And call untagSocket after transfer of data

TrafficStats.setThreadStatsTag(0xF00D);
TrafficStats.tagSocket(outputSocket);
// Transfer data using socket
TrafficStats.untagSocket(outputSocket);

How to make it with Coil?