Skip to content

Commit

Permalink
fix: fixed memory pools & race issues
Browse files Browse the repository at this point in the history 
* fixes
  * a memory leak with recycled *Result in schema_props.go was causing
  some invalid results when GC is under pressure (seen on go-swagger test)
    * refactored schema props to isolate anyOf, oneOf, allOf, etc.
    * made the validation logic a little clearer to follow
    * fixed leaks with unreleased allocated Results
    * ensured that child validators for schemaPropsValidator cannot be
      reused after they are exhausted
    * ensured that recyclable results are redeemed in all cases in default
     and example validators

  * with go1.22, the race detector figured a race condition when expanding
    the swagger schema for parameters (spec.go)
    * before iterating over parameters to validate the swagger schema for
      a parameter, deep clone the schema from the root spec. This is
      performed once for a spec, so the perf impact should be negligible

* tests
  * added test for go-swagger fixture #2866, which exhibits the failure
    with the highest probability
  * added a debug version of the pools (build with the "validatedebug"
    build tag) to assert more thorough checks of the correctness of the
    pools usage

Signed-off-by: Frederic BIDON <[email protected]>
  • Loading branch information
@fredbi
fredbi committed Mar 3, 2024
1 parent f563d1c commit 69f070e
Show file tree
Hide file tree
Showing 20 changed files with 11,116 additions and 338 deletions.
20 changes: 16 additions & 4 deletions default_validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func (d *defaultValidator) isVisited(path string) bool {

// Validate validates the default values declared in the swagger spec
func (d *defaultValidator) Validate() *Result {
errs := poolOfResults.BorrowResult() // will redeem when merged
errs := pools.poolOfResults.BorrowResult() // will redeem when merged

if d == nil || d.SpecValidator == nil {
return errs
Expand All @@ -97,7 +97,7 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result {
// every default value that is specified must validate against the schema for that property
// headers, items, parameters, schema

res := poolOfResults.BorrowResult() // will redeem when merged
res := pools.poolOfResults.BorrowResult() // will redeem when merged
s := d.SpecValidator

for method, pathItem := range s.expandedAnalyzer().Operations() {
Expand All @@ -119,6 +119,8 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result {
if red.HasErrorsOrWarnings() {
res.AddErrors(defaultValueDoesNotValidateMsg(param.Name, param.In))
res.Merge(red)
} else if red.wantsRedeemOnMerge {
pools.poolOfResults.RedeemResult(red)
}
}

Expand All @@ -128,6 +130,8 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result {
if red.HasErrorsOrWarnings() {
res.AddErrors(defaultValueItemsDoesNotValidateMsg(param.Name, param.In))
res.Merge(red)
} else if red.wantsRedeemOnMerge {
pools.poolOfResults.RedeemResult(red)
}
}

Expand All @@ -137,6 +141,8 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result {
if red.HasErrorsOrWarnings() {
res.AddErrors(defaultValueDoesNotValidateMsg(param.Name, param.In))
res.Merge(red)
} else if red.wantsRedeemOnMerge {
pools.poolOfResults.RedeemResult(red)
}
}
}
Expand Down Expand Up @@ -188,6 +194,8 @@ func (d *defaultValidator) validateDefaultInResponse(resp *spec.Response, respon
if red.HasErrorsOrWarnings() {
res.AddErrors(defaultValueHeaderDoesNotValidateMsg(operationID, nm, responseName))
res.Merge(red)
} else if red.wantsRedeemOnMerge {
pools.poolOfResults.RedeemResult(red)
}
}

Expand All @@ -197,6 +205,8 @@ func (d *defaultValidator) validateDefaultInResponse(resp *spec.Response, respon
if red.HasErrorsOrWarnings() {
res.AddErrors(defaultValueHeaderItemsDoesNotValidateMsg(operationID, nm, responseName))
res.Merge(red)
} else if red.wantsRedeemOnMerge {
pools.poolOfResults.RedeemResult(red)
}
}

Expand All @@ -216,6 +226,8 @@ func (d *defaultValidator) validateDefaultInResponse(resp *spec.Response, respon
// Additional message to make sure the context of the error is not lost
res.AddErrors(defaultValueInDoesNotValidateMsg(operationID, responseName))
res.Merge(red)
} else if red.wantsRedeemOnMerge {
pools.poolOfResults.RedeemResult(red)
}
}
return res
Expand All @@ -227,7 +239,7 @@ func (d *defaultValidator) validateDefaultValueSchemaAgainstSchema(path, in stri
return nil
}
d.beingVisited(path)
res := poolOfResults.BorrowResult()
res := pools.poolOfResults.BorrowResult()
s := d.SpecValidator

if schema.Default != nil {
Expand Down Expand Up @@ -273,7 +285,7 @@ func (d *defaultValidator) validateDefaultValueSchemaAgainstSchema(path, in stri
// TODO: Temporary duplicated code. Need to refactor with examples

func (d *defaultValidator) validateDefaultValueItemsAgainstSchema(path, in string, root interface{}, items *spec.Items) *Result {
res := poolOfResults.BorrowResult()
res := pools.poolOfResults.BorrowResult()
s := d.SpecValidator
if items != nil {
if items.Default != nil {
Expand Down
24 changes: 16 additions & 8 deletions example_validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func (ex *exampleValidator) isVisited(path string) bool {
// - individual property
// - responses
func (ex *exampleValidator) Validate() *Result {
errs := poolOfResults.BorrowResult()
errs := pools.poolOfResults.BorrowResult()

if ex == nil || ex.SpecValidator == nil {
return errs
Expand All @@ -75,7 +75,7 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result {
// in: schemas, properties, object, items
// not in: headers, parameters without schema

res := poolOfResults.BorrowResult()
res := pools.poolOfResults.BorrowResult()
s := ex.SpecValidator

for method, pathItem := range s.expandedAnalyzer().Operations() {
Expand All @@ -97,6 +97,8 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result {
if red.HasErrorsOrWarnings() {
res.AddWarnings(exampleValueDoesNotValidateMsg(param.Name, param.In))
res.MergeAsWarnings(red)
} else if red.wantsRedeemOnMerge {
pools.poolOfResults.RedeemResult(red)
}
}

Expand All @@ -106,8 +108,8 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result {
if red.HasErrorsOrWarnings() {
res.AddWarnings(exampleValueItemsDoesNotValidateMsg(param.Name, param.In))
res.Merge(red)
} else {
poolOfResults.RedeemResult(red)
} else if red.wantsRedeemOnMerge {
pools.poolOfResults.RedeemResult(red)
}
}

Expand All @@ -117,8 +119,8 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result {
if red.HasErrorsOrWarnings() {
res.AddWarnings(exampleValueDoesNotValidateMsg(param.Name, param.In))
res.Merge(red)
} else {
poolOfResults.RedeemResult(red)
} else if red.wantsRedeemOnMerge {
pools.poolOfResults.RedeemResult(red)
}
}
}
Expand Down Expand Up @@ -170,6 +172,8 @@ func (ex *exampleValidator) validateExampleInResponse(resp *spec.Response, respo
if red.HasErrorsOrWarnings() {
res.AddWarnings(exampleValueHeaderDoesNotValidateMsg(operationID, nm, responseName))
res.MergeAsWarnings(red)
} else if red.wantsRedeemOnMerge {
pools.poolOfResults.RedeemResult(red)
}
}

Expand All @@ -179,6 +183,8 @@ func (ex *exampleValidator) validateExampleInResponse(resp *spec.Response, respo
if red.HasErrorsOrWarnings() {
res.AddWarnings(exampleValueHeaderItemsDoesNotValidateMsg(operationID, nm, responseName))
res.MergeAsWarnings(red)
} else if red.wantsRedeemOnMerge {
pools.poolOfResults.RedeemResult(red)
}
}

Expand All @@ -198,6 +204,8 @@ func (ex *exampleValidator) validateExampleInResponse(resp *spec.Response, respo
// Additional message to make sure the context of the error is not lost
res.AddWarnings(exampleValueInDoesNotValidateMsg(operationID, responseName))
res.Merge(red)
} else if red.wantsRedeemOnMerge {
pools.poolOfResults.RedeemResult(red)
}
}

Expand Down Expand Up @@ -225,7 +233,7 @@ func (ex *exampleValidator) validateExampleValueSchemaAgainstSchema(path, in str
}
ex.beingVisited(path)
s := ex.SpecValidator
res := poolOfResults.BorrowResult()
res := pools.poolOfResults.BorrowResult()

if schema.Example != nil {
res.MergeAsWarnings(
Expand Down Expand Up @@ -271,7 +279,7 @@ func (ex *exampleValidator) validateExampleValueSchemaAgainstSchema(path, in str
//

func (ex *exampleValidator) validateExampleValueItemsAgainstSchema(path, in string, root interface{}, items *spec.Items) *Result {
res := poolOfResults.BorrowResult()
res := pools.poolOfResults.BorrowResult()
s := ex.SpecValidator
if items != nil {
if items.Example != nil {
Expand Down
Loading

0 comments on commit 69f070e

Please sign in to comment.