search

kubernetes / kube-openapi

Kubernetes OpenAPI spec generation & serving
Apache License 2.0
319 stars 208 forks source link

Make sure updating spec is not racy #385

Closed apelisse closed 1 year ago

apelisse commented 1 year ago

cc @soltysh @liggitt

apelisse commented 1 year ago

Still not sure how this would end-up being nil ...

apelisse commented 1 year ago

I've tried giving a nil swagger, and it fails when trying to serialize the json (as expected), not with a nil json ...

Next question is: if the swagger object changes causing a race, would it return a nil/empty json? I guess it's possible since that's probably undefined behavior.

Jefftree commented 1 year ago

@apelisse I don't think we need this anymore now that the cache is thread safe?

apelisse commented 1 year ago

We still have some fairly odd wiring here that maybe makes it worth it to check whether this is thread-safe. Also let me remove the mutex, it shouldn't be needed now.

apelisse commented 1 year ago

I've removed the lock and improved the race test a little bit, should be good to go now, PTAL.

Jefftree commented 1 year ago

/lgtm /approve /hold (if you want to fix up the last comment)

k8s-ci-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, Jefftree

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes/kube-openapi/blob/master/OWNERS)~~ [Jefftree,apelisse] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
apelisse commented 1 year ago

Appended Chan to all the channels in that test.

Jefftree commented 1 year ago

/lgtm /unhold