swiftkube / client

Swift client for Kubernetes
Apache License 2.0
129 stars 20 forks source link

Client crashes `GenericKubernetesClient.prepareDecoder` #24

Closed t089 closed 1 year ago

t089 commented 1 year ago

Hello, not sure if this helps, but I noticed a crash in an app today that might be related to SwiftkubeClient. This is the backtrace:

Received signal 11. Backtrace:
0x56295651b302, Backtrace.(printBacktrace in _B82A8C0ED7C904841114FDF244F9E58E)(signal: Swift.Int32) -> () at /src/.build/checkouts/swift-backtrace/Sources/Backtrace/Backtrace.swift:66
0x56295651b597, closure #1 (Swift.Int32) -> () in static Backtrace.Backtrace.install(signals: Swift.Array<Swift.Int32>) -> () at /src/.build/checkouts/swift-backtrace/Sources/Backtrace/Backtrace.swift:80
0x56295651b597, @objc closure #1 (Swift.Int32) -> () in static Backtrace.Backtrace.install(signals: Swift.Array<Swift.Int32>) -> () at /src/<compiler-generated>:79
0x7f11683f413f
0x56295799cb21
0x5629568fd5de, generic specialization <Swift.CodingUserInfoKey, Any> of Swift.Dictionary._Variant.setValue(_: __owned B, forKey: A) -> () at /src/<compiler-generated>:0
0x5629568fd5de, generic specialization <Swift.CodingUserInfoKey, Any> of Swift.Dictionary.subscript.setter : (A) -> Swift.Optional<B> at /src/<compiler-generated>:0
0x5629568fd534, SwiftkubeClient.GenericKubernetesClient.prepareDecoder(Foundation.JSONDecoder) -> () at /src/.build/checkouts/client/Sources/SwiftkubeClient/Client/GenericKubernetesClient.swift:76
0x5629568fd84a, protocol witness for SwiftkubeClient.RequestHandlerType.prepareDecoder(Foundation.JSONDecoder) -> () in conformance SwiftkubeClient.GenericKubernetesClient<A> : SwiftkubeClient.RequestHandlerType in SwiftkubeClient at /src/<compiler-generated>:0
0x56295690b287, (5) suspend resume partial function for (extension in SwiftkubeClient):SwiftkubeClient.RequestHandlerType.dispatch<A where A1: Swift.Decodable>(request: SwiftkubeClient.KubernetesRequest, expect: A1.Type) async throws -> A1 at /src/.build/checkouts/client/Sources/SwiftkubeClient/Client/RequestHandlerType.swift:78
0x56295768befd
0x56295768c5eb
0x562957652694
0x562957652442
0x56295765dd31
0x7f11683e8ea6
0x7f11680f8a2e
0xffffffffffffffff

At this time the app was most likely doing only one thing: watching a list of Pods.

let watchTask = try self.client.pods.watch(in: ns, options: options) { [logger] event, pod in
  logger.debug("event: \(event): pod: \(pod.name ?? "")")
  if event == .error {
    // we need to restart the watcher
    Task {
      try? self.observePods()
    }
  } else {
    Task {
      try await self.refreshTargets()
    }
  }
}

And refreshTargets only does this with client:

let listOfPods = try await self.client.pods.list(in: ns, options: options)
Client Version: version.Info{Major:"1", Minor:"23", GitVersion:"v1.23.13", GitCommit:"592eca05be27f7d927d0b25cbb4241d75a9574bf", GitTreeState:"clean", BuildDate:"2022-10-12T10:57:16Z", GoVersion:"go1.17.13", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"23+", GitVersion:"v1.23.14-eks-ffeb93d", GitCommit:"96e7d52c98a32f2b296ca7f19dc9346cf79915ba", GitTreeState:"clean", BuildDate:"2022-11-29T18:43:31Z", GoVersion:"go1.17.13", Compiler:"gc", Platform:"linux/amd64"}
    {
      "identity" : "client",
      "kind" : "remoteSourceControl",
      "location" : "https://github.com/swiftkube/client.git",
      "state" : {
        "revision" : "6809c6aeff28be17afe82e6e0287e1b8c9d38ee1",
        "version" : "0.12.0"
      }
    },
    {
      "identity" : "model",
      "kind" : "remoteSourceControl",
      "location" : "https://github.com/swiftkube/model.git",
      "state" : {
        "revision" : "3f40e3e7e4f8202c88ad3605e85ad98eba198786",
        "version" : "0.6.0"
      }
    }
t089 commented 1 year ago

I wonder if the userInfo dictionary of JSONDecoder is thread-safe. What happens when the prepareDecoder method is called concurrently for a shared client?

t089 commented 1 year ago

Hm this looks suspicious: The JSONDecoder seems to be shared with all requests handled by the client, which is fine, but the problem might be that the userInfo dict is being mutated in RequestHandlerType.dispatch<T: Decodable>(request: KubernetesRequest, expect responseType: T.Type) async throws -> T when it calls perpareDecoder(jsonDecoder).

iabudiab commented 1 year ago

@t089 Thanks for the bug report 👍 It seem, like you're onto something. The thread-safety of the userInfo is very likely to be the culprit here. I guess, it won't be easy to reproduce 🤔

I'll try to find a solution for propagating the userInfo in an async context.

iabudiab commented 1 year ago

One fix for this would be the no-code approach 😉. The userInfo dictionary is only required for decoding an unknown type to a type-erased AnyKubernetesAPIResource.

I was planning on dropping the AnyKubernetesAPIResource because it was superseded by the UnstructuredResource in SwiftkubeModel v0.5.0. Its usage is demonstrated in the swiftkube-c-t-l example here

However, there are some missing bits and pieces, in order to completely make the transition.

t089 commented 1 year ago

other quick fixes:

iabudiab commented 1 year ago

Yes, however, there are some drawbacks with keeping that code, since it was more-or-less a workaround for decoding unknown types. With the introduction of GroupVersionResouce, UnstructuredResource and derivatives in v0.5.0, there is no need anymore for this workaround.

Besides, I think it is better to avoid creating a decoder for each request, in order to be more memory friendly. And a global lock would be another workaround.

t089 commented 1 year ago

Yes, sounds reasonable!