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 data race in error path #4

Merged
merged 2 commits into from
Jan 24, 2023
Merged

Fix data race in error path #4

merged 2 commits into from
Jan 24, 2023

Conversation

larsve
Copy link
Contributor

@larsve larsve commented Jan 24, 2023

Fixes a data race in ToMiddleware() by creating a new instance of the err variable in the returned function.

Fixes a data race in ToMiddleware() by creating a new instance of the err
variable in the returned function.
@aldas
Copy link
Contributor

aldas commented Jan 24, 2023

@larsve could you provide testcase showing this race. I struggling to understand how this can be racy. Is it because that error instance is created at extractors, err := CreateExtractors(config.TokenLookup)?

@larsve
Copy link
Contributor Author

larsve commented Jan 24, 2023

Yes, I hit that issue when GO's race detector broke my integration test, complaining about:

WARNING: DATA RACE
Write at 0x00c0002aa760 by goroutine 105:
github.com/labstack/echo-jwt/v4.Config.ToMiddleware.func2.1()
/go/pkg/mod/github.com/labstack/echo-jwt/v4@v4.0.0/jwt.go:217 +0x264

And se far as I could see, it complained because the returned function is reusing the err instance that is created in the ToMiddleware() func. With this fix, the race detector isn't triggered anymore.

@larsve
Copy link
Contributor Author

larsve commented Jan 24, 2023

As I understand the issue, it's when there are concurrent calls to the returned function, since every call to the returned function is reusing the same err variable.

@larsve
Copy link
Contributor Author

larsve commented Jan 24, 2023

Added a small testcase now, if I revert my fix, I get this when running go test -race ./...:

Write at 0x00c00021a990 by goroutine 49:                                                                                                
  github.com/labstack/echo-jwt/v4.Config.ToMiddleware.func2.1()
      /home/lars-erik/proj/echo-jwt/jwt.go:228 +0x264
  github.com/labstack/echo-jwt/v4.TestToMiddlewareRace.func4()
      /home/lars-erik/proj/echo-jwt/jwt_test.go:776 +0xda
                                                                    
Previous write at 0x00c00021a990 by goroutine 58:
  github.com/labstack/echo-jwt/v4.Config.ToMiddleware.func2.1()
      /home/lars-erik/proj/echo-jwt/jwt.go:228 +0x264
  github.com/labstack/echo-jwt/v4.TestToMiddlewareRace.func4()   
      /home/lars-erik/proj/echo-jwt/jwt_test.go:776 +0xda

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aldas aldas merged commit 296613d into labstack:main Jan 24, 2023
@aldas
Copy link
Contributor

aldas commented Jan 24, 2023

I'll release patch release asap

@aldas aldas changed the title Fix data race in Config.ToMiddleware.func2.1() Fix data race in error path Jan 24, 2023
@aldas
Copy link
Contributor

aldas commented Jan 24, 2023

Thank you @larsve and v4.0.1 is out

@larsve larsve deleted the fix_data_race branch January 24, 2023 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants