Use a local keyword registry to make validate thread-safe #103
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce a keyword registry struct so that most operations are not acting upon a global package variable. I have preserved existing behaviors (such as applying the default schema to the global keyword registry), and kept global functions such as RegistrKeyword. If breaking changes are allowed, then we could remove the locks and keyword registry copying which would simplify this patch quite a bit. Signed-off-by: Carolyn Van Slyck <[email protected]>
carolynvs
force-pushed
the
local-keyword-registry2
branch
from
a1c05f4 to
b7bc812
Compare
|
Again, very appreciated. This will take a bit more time to review as I have to be more thorough with it and do some testing, but at a glance looks like a step in the right direction. |
|
@Arqu Just checking if you've had a chance to look at this and had any feedback? |
|
Sorry, been pretty booked for the past while. I'll aim to have a go at it within the next 10 days. |
Arqu
requested changes
Jul 22, 2021
There was a problem hiding this comment.
Thank you for the patience, this is exceptional and brings us much closer to being fully thread safe. Before merging, if you could just address the golint issues
draft2019_09_keywords.go:14:1: exported method KeywordRegistry.DefaultIfEmpty should have comment or be unexported
keyword.go:31:6: exported type KeywordRegistry should have comment or be unexported
keyword.go:55:1: exported method KeywordRegistry.Copy should have comment or be unexported
And I'm happy to merge as is.
Signed-off-by: Carolyn Van Slyck <[email protected]>
carolynvs
force-pushed
the
local-keyword-registry2
branch
from
4ede4ee to
39ced0c
Compare
|
Oh great! I'm excited to not have to have a fork of jsonschema anymore! 🎉 I just pushed a fix for the golint errors. |
Arqu
approved these changes
Jul 23, 2021
carolynvs
added a commit
to carolynvs/cnab-go
that referenced
this pull request
Jul 23, 2021
The PR fixing thread safety when registering a custom keyword has been merged so I'm removing the fork and going back to using the upstream repository. See qri-io/jsonschema#103 Signed-off-by: Carolyn Van Slyck <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes validating a schema thread-safe. I was focused on fixing that call path specifically, and not addressing thread-safety throughout the entire code base, so there is may be more global variables being used elsewhere. This PR introduces a a struct to represent a keyword registry, and loading of the registry and validation occurs in instances of a keyword registry, avoiding the use of global package variables.
This is the patch that we have been using for https://github.com/getporter/porter for 6+ months to be able to validate a schema in a thread-safe manner when registering custom validators.
I was mostly interested in getting this to work without making any backwards incompatible changes to this library's existing API. For example, I kept the package level functions that manipulate package variables, such as
RegisterKeyword, which requires the use of locks. If you are okay with making a breaking change, and removing global state, that would simplify the patch quite a bit.This is a fix for #80 , but is limited to validation. There are probably more spots that would need to be refactored as well.