-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Make getIssueFullPattern() concurrency save - bugfix #16415 (#16155, #16185) #16416
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
Make getIssueFullPattern() concurrency save - bugfix #16415 (#16155, #16185) #16416
Conversation
@Dexus can you please target master first and then we will backport stuff if merged? |
@@ -70,9 +70,6 @@ var ( | |||
// CSS class for action keywords (e.g. "closes: #1") | |||
const keywordClass = "issue-keyword" | |||
|
|||
// regexp for full links to issues/pulls | |||
var issueFullPattern *regexp.Regexp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would propose a sync.Once loock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh no you dont need to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably an antipattern but the init() function would also work, wouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes is init is an antipattern ...
var issueFullPattern = regexp.MustCompile(regexp.QuoteMeta(setting.AppURL) + | ||
`\w+/\w+/(?:issues|pulls)/((?:\w{1,10}-)?[1-9][0-9]*)([\?|#]\S+.(\S+)?)?\b`) | ||
|
||
return func() *regexp.Regexp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offtopic and I don't know go but would this really be needed (creating and calling this function)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no did just create a pull
see #16417 |
I'm glad it's fixed, even if it's a pity that you're deprived of your own contribution. |
@Dexus I thought about creating the diff and send it to you ... but it was easyer to just submit a pull then ... I of cousre can mention you in the pull if you like beside you did targed the wrong branch :D |
That's not my point at all. Find it only a pity and wanted to put it also so to the expression. Anyway, thanks that it was addressed. |
@Dexus as per #16417 (comment) I think you can have the cance to a pull now ;) just use sync.Once to init it on first call to the func |
Run processors on whole of text (#16155) #16185
This PR makes the function concurrent save. Fix the mention in PR #16185 and merge 5ff807a
Fixes #16415