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

Client crashes GenericKubernetesClient.prepareDecoder #24

Closed
t089 opened this issue Jan 25, 2023 · 7 comments
Closed

Client crashes GenericKubernetesClient.prepareDecoder #24

t089 opened this issue Jan 25, 2023 · 7 comments
Labels
bug Something isn't working

Comments

@t089
Copy link
Contributor

t089 commented Jan 25, 2023

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
Copy link
Contributor Author

t089 commented Jan 25, 2023

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
Copy link
Contributor Author

t089 commented Jan 25, 2023

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
Copy link
Member

@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 iabudiab added the bug Something isn't working label Jan 25, 2023
@iabudiab iabudiab changed the title Crash report Client crashes GenericKubernetesClient.prepareDecoder Jan 25, 2023
@iabudiab
Copy link
Member

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
Copy link
Contributor Author

t089 commented Jan 25, 2023

other quick fixes:

  • do not share the JSONDecoder but create a new one for each request.
  • continue to share, but protect the access to the unserInfo with a global lock (should be mostly fine unless there are a TON of concurrent requests within the same process).

@iabudiab
Copy link
Member

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
Copy link
Contributor Author

t089 commented Jan 25, 2023

Yes, sounds reasonable!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants