Thread Performance Warning Due to Priority Inversion when Initializing NIO Client

[mycroftcanner]

Description:

When initializing the Connect client, the following warning arises:

Thread Performance Checker: Thread running at User-initiated quality-of-service class waiting on a lower QoS thread running at Default quality-of-service class. Investigate ways to avoid priority inversions.
PID: 7210, TID: 8927

This warning appears to be related to this issue in the swift-nio repository. It seems connect-swift might be impacted by the underlying problem with swift-nio.

Expected Behavior: No thread performance warnings should be present when initializing the Connect client.

Steps to Reproduce:

1. Initialize the Connect client in a Swift project.
2. Observe warnings during runtime.

Environment Details:

• Connect-Swift version: 0.8.0
• Xcode 15

[rebello95]

Thanks for flagging this @mycroftcanner - yes, it looks like this is an underlying issue with NIO which wants to force MultiThreadedEventLoopGroup to be instantiated off the main thread. I'm curious to see what they say in response to your comment on the issue you linked. Having Connect manually push NIO instantiation to a separate thread would be tricky since this happens on initialization. One thing you could do is instantiate your ProtocolClient and/or NIOHTTPClient on a background thread instead of main and I think that would resolve this warning

[mycroftcanner]

I tried instantiating the client on a background thread and it doesn't work.. and I can't figure out how to specify the event group loop. I tried to instantiate the NIOHTTPCLient and that also failed.

This is how I instantiate my client... and I am on a user initiated background thread when this happens... doing it on a background thread with qos: .default or background doesn't help at all.

    let client = Search_V1_SearchServiceClient(
      client: ProtocolClient(
        httpClient: NIOHTTPClient(host: searchURL, port: 443),
        config: ProtocolClientConfig(
          host: searchURL,
          networkProtocol: .connect,
          codec: ProtoCodec(),
          requestCompression: .init(minBytes: 50_000, pool: GzipCompressionPool())
        )
      )
    )

[rebello95]

@mycroftcanner I'm able to silence the warning when instantiating NIOHTTPClient from an async background thread. For example:

// No warning
DispatchQueue.global(qos: .background).async {
    _ = NIOHTTPClient(host: "https://www.connectrpc.com")
}

// Warning
DispatchQueue.main.async {
    _ = NIOHTTPClient(host: "https://www.connectrpc.com")
}

Note that doing DispatchQueue.global(qos: .background).sync (not async) will still warn due to the fact that the main thread must wait on the background thread. Does this work for you?

[mycroftcanner]

I appreciate the effort, but it wasn't useful in my scenario. I needed the client to be accessible beyond the background thread.

That said, the following actor-based setup worked for me:

  actor SearchClient {
    let client: Search_V1_SearchServiceClient
    
    init(searchURL: String) async {
      self.client = .init(
        client: ProtocolClient(
          httpClient: await Task(priority: .background) {
            NIOHTTPClient(host: searchURL, port: 443)
          }.value,
          config: ProtocolClientConfig(
            host: searchURL,
            networkProtocol: .connect,
            codec: ProtoCodec(),
            requestCompression: .init(minBytes: 50_000, pool: GzipCompressionPool())
          )
        )
      )
    }

[mycroftcanner]

Initialization of the actor was more complex, as it couldn't be done synchronously:

 actor SearchClient {
    private var _client: Search_V1_SearchServiceClient?
    private let searchURL: String
    
    init(searchURL: String) {
      self.searchURL = searchURL
    }
    
    var client: Search_V1_SearchServiceClient {
      get async {
        if _client == nil {
          _client = .init(
            client: ProtocolClient(
              httpClient: await Task(priority: .background) {
                NIOHTTPClient(host: searchURL, port: 443)
              }.value,
              config: ProtocolClientConfig(
                host: searchURL,
                networkProtocol: .connect,
                codec: ProtoCodec(),
                requestCompression: .init(minBytes: 50_000, pool: GzipCompressionPool())
              )
            )
          )
        }
        return _client!
      }
    }

    func profiles(request: ProfilesRequest, headers: Connect.Headers) async throws -> ProfilesResponse {
      let response = await client.profiles(request: request, headers: headers)
      if let error = response.error { throw error }
      return response.message ?? .init()
    }    
  }

Honestly, users of Connect Swift shouldn't have to navigate these complexities.

[rebello95]

I'm glad you got it working. I'll keep this issue open so I can investigate how to manually force NIO initialization to happen on a background thread from within Connect

[rebello95]

I spent a bit of time looking into this, but forcing the MultiThreadedEventLoopGroup to be instantiated from a background thread within Connect is non-trivial because the unary and stream functions need to access that synchronously to return, which means either the initializer for NIOHTTPClient would need to block or those functions would need to block. For now, I think the best approach is to use the same approach that was suggested for working around this in SwiftGRPC (instantiating the client from the background): apple/swift-nio#2223 (comment)

[rebello95]

Sounds like a fix will be going into NIO based on the thread linked above, so we'll wait on that.

[mycroftcanner]

FYI apple/swift-nio#2620

[rebello95]

Awesome! Will update our NIO dependency once they tag a new release upstream.

[rebello95]

#240