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

fix: fix memory pools & race issues #178

Merged
merged 1 commit into from
Mar 3, 2024
Merged

Conversation

fredbi
Copy link
Member

@fredbi fredbi commented Feb 27, 2024

  • 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]

Copy link

codecov bot commented Feb 27, 2024

Codecov Report

Attention: Patch coverage is 87.38462% with 41 lines in your changes are missing coverage. Please review.

Project coverage is 92.53%. Comparing base (fb01d6d) to head (7b1fec1).
Report is 4 commits behind head on master.

Files Patch % Lines
schema_props.go 86.06% 9 Missing and 8 partials ⚠️
spec.go 78.57% 3 Missing and 3 partials ⚠️
validator.go 90.19% 4 Missing and 1 partial ⚠️
default_validator.go 75.00% 2 Missing and 2 partials ⚠️
example_validator.go 75.00% 2 Missing and 2 partials ⚠️
result.go 50.00% 3 Missing ⚠️
formats.go 66.66% 1 Missing ⚠️
slice_validator.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #178      +/-   ##
==========================================
- Coverage   93.30%   92.53%   -0.77%     
==========================================
  Files          23       23              
  Lines        3839     3150     -689     
==========================================
- Hits         3582     2915     -667     
+ Misses        203      154      -49     
- Partials       54       81      +27     
Flag Coverage Δ
oldstable 92.47% <87.38%> (-0.83%) ⬇️
stable 92.53% <87.38%> (-0.77%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fredbi fredbi changed the title repro: attempt to reproduce bug on windows with 2866.yaml fix: fix memory leak & race issues Feb 28, 2024
@fredbi fredbi marked this pull request as ready for review February 28, 2024 11:07
* 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]>
@fredbi fredbi changed the title fix: fix memory leak & race issues fix: fix memory pools & race issues Mar 3, 2024
@fredbi fredbi merged commit 69f070e into go-openapi:master Mar 3, 2024
9 of 11 checks passed
@fredbi fredbi deleted the repro/2866 branch March 3, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant