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 ANTLR race condition #17

Closed
plorenz opened this issue Jan 7, 2020 · 8 comments
Closed

Fix ANTLR race condition #17

plorenz opened this issue Jan 7, 2020 · 8 comments

Comments

@plorenz
Copy link
Member

plorenz commented Jan 7, 2020

Create and submit patch for antlr/antlr4#2040, which the go race detector finds in our code.

@jaystarshot
Copy link

jaystarshot commented Mar 30, 2020

Can I try to fix this issue?

@plorenz
Copy link
Member Author

plorenz commented Mar 30, 2020

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
Copy link

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

@plorenz
Copy link
Member Author

plorenz commented Mar 31, 2020

@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
Copy link

@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
Copy link
Member Author

plorenz commented Mar 31, 2020

@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: 9db626f

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
Copy link

@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
Copy link
Member Author

plorenz commented Jan 14, 2021

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

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

No branches or pull requests

2 participants