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

Stop using MustCompile, and instead return an error if any regex fails to compile #67

Open
veqryn opened this issue Jan 29, 2020 · 4 comments

Comments

@veqryn
Copy link

veqryn commented Jan 29, 2020

The use of regex.MustCompile (see:

uap-go/uaparser/parser.go

Lines 354 to 360 in daf92ba

func compileRegex(flags, expr string) *regexp.Regexp {
if flags == "" {
return regexp.MustCompile(expr)
} else {
return regexp.MustCompile(fmt.Sprintf("(?%s)%s", flags, expr))
}
}
) means that if any regex fails, there is a panic.

We pull down the master regex url daily, so that our regexes are always up to date, and load it right into our servers. The panic cause all our servers to crash last night, until we pinned the master regex url to a working version.

NewFromBytes already has the option to return an error, so why not return the error there?

uap-go/uaparser/parser.go

Lines 209 to 219 in daf92ba

func NewFromBytes(data []byte) (*Parser, error) {
var definitions RegexesDefinitions
if err := yaml.Unmarshal(data, &definitions); err != nil {
return nil, err
}
parser := &Parser{definitions, 0, 0, 0, (EOsLookUpMode|EUserAgentLookUpMode|EDeviceLookUpMode), false, false}
parser.mustCompile()
return parser, nil
}

@sylvain101010
Copy link

sylvain101010 commented Jul 29, 2020

I agree!
@elsigh would you consider a merge request changing the panics to correct error handling (also in NewFromSaved)?

(it's a breaking change)

@yehaotong
Copy link

regex.MustCompile 的使用(见:

uap-go/uaparser/parser.go

Lines 354 to 360 in daf92ba

func compileRegex(flags, expr string) *regexp.Regexp {
if flags == "" {
return regexp.MustCompile(expr)
} else {
return regexp.MustCompile(fmt.Sprintf("(?%s)%s", flags, expr))
}
}

) 意味着如果任何正则表达式失败,就会出现恐慌。
我们每天下拉主正则表达式网址,以便我们的正则表达式始终是最新的,并将其直接加载到我们的服务器中。恐慌导致我们所有的服务器昨晚崩溃,直到我们将主正则表达式 url 固定到一个工作版本。

NewFromBytes 已经可以选择返回错误,那么为什么不在那里返回错误呢?

uap-go/uaparser/parser.go

Lines 209 to 219 in daf92ba

func NewFromBytes(data []byte) (*Parser, error) {
var definitions RegexesDefinitions
if err := yaml.Unmarshal(data, &definitions); err != nil {
return nil, err
}
parser := &Parser{definitions, 0, 0, 0, (EOsLookUpMode|EUserAgentLookUpMode|EDeviceLookUpMode), false, false}
parser.mustCompile()
return parser, nil
}

I want to ask you if this error will cause the program to hang up directly without any error log. My program also uses client: = parser.parse, and hang up directly without any error log during operation, which makes me wonder where there is a bug

@elsigh
Copy link
Contributor

elsigh commented Aug 24, 2021

I'm happy to accept a breaking change, but one issue really is that I'm not using this library in prod atm so I'd rather find someone else who can manage/own such a change.. Interested @skerkour ?

@sylvain101010
Copy link

Hello,
Unfortunately I absolutely don't have the bandwidth available to own the change.

Kind regards

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

4 participants