search

openziti / foundation

Foundation components for the Ziti golang ecosystem
Apache License 2.0
24 stars 4 forks source link

Fix ANTLR race condition #17

Closed plorenz closed 3 years ago

plorenz commented 4 years ago

Create and submit patch for https://github.com/antlr/antlr4/issues/2040, which the go race detector finds in our code.

jaystarshot commented 4 years ago

Can I try to fix this issue?

plorenz commented 4 years ago

Can I try to fix this issue?

Hi @carbylamine we'd be happy to have some help on this issue. As I think you've already aware, the fix needs to be to ANLTR4 for the linked issue: antlr/antlr4#2040. I'm assuming the ANLTR project would take a fix, if one were presented. If there's anything a Ziti dev can do to help, let us know.

jaystarshot commented 4 years ago

@plorenz herePlease check my comment if this is the same issue

plorenz commented 4 years ago

@plorenz herePlease check my comment if this is the same issue

I'm not sure I saw any bugs caused by race conditions, but I did see the Go race detector reporting issues so I fixed our generated code manually just to be safe. However, it's a pain to redo every time the grammar changes, so at some point I'm hoping ANTLR will either put out a fix or I'll have time to submit one.

jaystarshot commented 4 years ago

@plorenz, did you make the shared variables like lexerAtn,lexerDecisionToDFA etc local to each thread? Can you give a small example of your fix? Also, why don't you use a makefile and try to edit the generated files automatically?

plorenz commented 4 years ago

@plorenz, did you make the shared variables like lexerAtn,lexerDecisionToDFA etc local to each thread? Can you give a small example of your fix? Also, why don't you use a makefile and try to edit the generated files automatically?

@carbylamine yes, I followed the same approach someone else had taken and made the state per parser/lexer. Can see the change here: https://github.com/netfoundry/ziti-foundation/pull/43/commits/9db626f176ef88559d1008cf0dace5b3cefad304

I spent a few minutes looking to see if the fix could be scripted, but since the fix is relatively quick to apply and the grammar changes infrequently I didn't try for very long.

jaystarshot commented 4 years ago

@plorenz Cool thanks a lot, I will go ahead on this fix and work my way back to improve ANTLR once I get time, Thank you!

plorenz commented 3 years ago

FYI: @jaystarshot this is now fixed in antlr. See here: https://github.com/antlr/antlr4/pull/2816 for a discussion on how to mitigate the performance impacts