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

Report a warning if a rule using a 3rd party service timeout. #627

Closed
sarvaje opened this issue Nov 3, 2017 · 5 comments
Closed

Report a warning if a rule using a 3rd party service timeout. #627

sarvaje opened this issue Nov 3, 2017 · 5 comments

Comments

@sarvaje
Copy link
Contributor

sarvaje commented Nov 3, 2017

Some of the errors we get with sonar is a timeout when using a 3rd party service in a rule.

We should prevent sonar crash because of that(as we do with axe) and report a warning instead.

@molant
Copy link
Member

molant commented Nov 3, 2017

We have to do that in the following rules:

@qzhou1607-zz
Copy link
Contributor

Investigation on why this following timeout didn't stop sonar from crash in the case of ssllabs:
https://github.com/sonarwhal/sonar/blob/ca02a33311e9dc3c17721bd0b916337b5d3617a9/src/lib/sonar.ts#L172-L198

The timeout here is not per rule, but per event. In the cases of some urls, resource loading takes extra long and because the maxRunTime in the online-service worker is fixed (180000 by default), the allowed time for resolving the ssllabs promise is actually shorter than the rulesTimeout. So sonar is terminated before the rule times out. The plan is to add a timeout inside ssllabs, but which value to use is the tricky part since we don't have control over how long the resource loading is going to take.

@molant
Copy link
Member

molant commented Nov 3, 2017

@qzhou1607 so the problem is with the online-scanner, right? Maybe we should change error to warning if we have to kill the process. @sarvaje, thoughts?

@qzhou1607-zz
Copy link
Contributor

The problem is that the rule didn't time out in sonar before time out in the online-service. We can make it time out in sonar sooner (such as in ssllabs we add a timeout with a shorter waiting time like 1 min), or have the jobRunTime in the online-service larger to make up for the extra time consumed in loading resources. But neither solution seems to work in all scenarios.

@qzhou1607 so the problem is with the online-scanner, right?

We can handle TIMEOUT errors in online-service differently so that the job is not labeled as error. But that still leaves the issue that ssllabs doesn't have enough time to run before the process is killed. Maybe we can make jobRunTime for the jobs configurable in the future. So if certain rules such as ssllabs timeout, let the user know and the user can choose to extend the waiting time for the job?

Maybe we should change error to warning if we have to kill the process.

@sarvaje
Copy link
Contributor Author

sarvaje commented Nov 3, 2017

I think we need to have a jobRunTime with a value enough to don't fail frequently, but it has to be a reasonable time, because we can't block a worker forever. We can't control if a site is slow or not, so, at some point, we need to abort the execution.

Maybe instead of set the job as error, if we reach the jobRunTime we can set the status of the rules as warning with a message like Sonar didn't return the result faster enough. Please try later and if the problem continue, contact us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants