search

cnabio / cnab-go

A Go implementation of CNAB Core 1.0
MIT License
69 stars 37 forks source link

Make validating a bundle thread-safe #237

Closed carolynvs closed 3 years ago

carolynvs commented 3 years ago

Background: The jsonschema library we use is not thread-safe: https://github.com/qri-io/jsonschema/issues/80. This prevents callers from validating and working with bundles concurrently.

Current Fix: I made a fork of the latest version jsonschema and tweaked it so that it doesn't use package variables for everything, and uses a lock when it does access the global schema. I'm not sure if that is a solution that they will merge, still waiting on a reply. We are still using unreleased commits on docker repos so at least there is precedent. 😀

The other changes you see in the PR are due to changes in the latest version of jsonschema.

I tried to fix this without changing jsonschema and wasn't successful. However I am not sure how we feel about using a fork of jsonschema when the patch may never get merged, or the upstream bug fixed.

We really needed this in Porter, so either way I'm maintaining the fork... 🤷‍♀️

radu-matei commented 3 years ago

I assume there are not other JSON schema libraries that are thread-safe?

Do you have any preference to maintaining the fork in your account, or in this organization? (I'm hoping you wouldn't be the only one responsible for updating it if it were in this organization.)

carolynvs commented 3 years ago

The other library is https://github.com/xeipuuv/gojsonschema. They don't say that they are threadsafe, there are still open issues with people asking about that or reporting errors (without reproduction). I like the idea of trying it out and seeing if it would work for us. Let me give that a try and see if it blows up or not.

carolynvs commented 3 years ago

Update: Both libraries need more patches to address threadsafety and also neither is making progress towards accepting any patches.

Another cnab-go user, @dataoleg, has run into problems with this bug. Since the "better" fix than this patch isn't forthcoming, I would like to see us merge this PR here so that at least cnab-go users are no longer affected.