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

[#60] Proper error handling #65

Merged
merged 1 commit into from Jan 19, 2018

Conversation

Projects
None yet
2 participants
@stereobooster
Collaborator

stereobooster commented Jan 6, 2018

Before

./bin/minimalcss.js http://127.0.0.1:8080/jserror.html
(node:7147) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Error: unhandled
    at http://127.0.0.1:8080/jserror.html:7:19
(node:7147) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

After

./bin/minimalcss.js http://127.0.0.1:8080/jserror.html
Error: Error: unhandled
    at http://127.0.0.1:8080/jserror.html:7:19
    at Page._handleException (~/minimalcss/node_modules/puppeteer/lib/Page.js:370:38)
    at Session.Page.client.on.exception (~/minimalcss/node_modules/puppeteer/lib/Page.js:95:60)
    at emitOne (events.js:115:13)
    at Session.emit (events.js:210:7)
    at Session._onMessage (~/minimalcss/node_modules/puppeteer/lib/Connection.js:210:12)
    at Connection._onMessage (~/minimalcss/node_modules/puppeteer/lib/Connection.js:105:19)
    at emitOne (events.js:115:13)
    at WebSocket.emit (events.js:210:7)
    at Receiver._receiver.onmessage (~/minimalcss/node_modules/ws/lib/WebSocket.js:143:47)
    at Receiver.dataMessage (~/minimalcss/node_modules/ws/lib/Receiver.js:389:14)
@peterbe

It certainly is a huge improvement. Thank you!

But there's one thing I don't understand. I tried this:

const minimalcss = require('./index')

minimalcss.minimize({urls: ['http://127.0.0.1:8080/jserror.html']}).then(result => {
  console.log('OUTPUT', result.finalCss.length, result.finalCss)
}).catch(error => {
  console.error(`Failed the minimize CSS: ${error}`)
})

(which is basically the sample code in the README).
Now, thanks to this PR, that .catch() the Failed the minimize CSS: stuff is printed out.

However, when I run this:

▶ node --trace-warnings dummy.js
Failed the minimize CSS: Error: Error: unhandled
    at http://127.0.0.1:8080/jserror.html:7:19


...HERE'S THE HUGE DELAY.
Presumably whilst waiting for puppeteer to do something about Chromium.
...



(node:19444) Error: Navigation Timeout Exceeded: 30000ms exceeded
    at Promise.then (/Users/peterbe/dev/JAVASCRIPT/minimalcss/node_modules/puppeteer/lib/NavigatorWatcher.js:69:21)
    at <anonymous>
(node:19444) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
    at emitWarning (internal/process/promises.js:69:15)
    at emitPendingUnhandledRejections (internal/process/promises.js:86:11)
    at runMicrotasksCallback (internal/process/next_tick.js:124:9)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickCallback (internal/process/next_tick.js:180:9)

I put in a console.log into your code and sure enough that browser.close() does get called. And I put a console.log after that line too. And it gets called almost immediately, and then the 30s "hang" starts.

src/run.js Outdated
// We can close the browser now that all URLs have been opened.
if (!options.browser) {
browser.close()
browser = null

This comment has been minimized.

@peterbe

peterbe Jan 12, 2018

Owner

This needs a comment that explains why the browser variable is changed.

This comment has been minimized.

@stereobooster

stereobooster Jan 12, 2018

Collaborator

I suppose there is better way to do this, what is basically required is finally to make sure we close browser, instead I'm using browser=null as a flag that browser already closed. Probably need to fix this

src/run.js Outdated
resolve(returned)
}).catch(function(reason) {
if (!options.browser && browser) {
browser.close()

This comment has been minimized.

@peterbe

peterbe Jan 12, 2018

Owner

Why is there no browser = null here?

@peterbe

This comment has been minimized.

Owner

peterbe commented Jan 12, 2018

I'm not sure I'm testing this right. But I'm tempted to land this as a good start.
The ideal thing would be that the code inside the catch() runs "immediately" and then the whole program exist also "immediately".

@stereobooster

This comment has been minimized.

Collaborator

stereobooster commented Jan 12, 2018

I will look into this 30s problem. Do not merge just yet

@stereobooster

This comment has been minimized.

Collaborator

stereobooster commented Jan 13, 2018

page.on('pageerror', error => {
  throw error;
})

^ This doesn't work because an error is thrown outside of the stack.

page.on('pageerror', error => {
  return reject(error)
})

^ This doesn't work because we reject Promise, but do not stop the main function - eventually, main function will finish and Promise will be rejected again or accepted, which is wrong.

We can do something like this:

let pageerror;

page.on('pageerror', error => {
  pageerror = error
})

...

if (pageerror) {
  return Promise.reject(pageerror)
} else {
  return Promise.resolve(...)
}
@stereobooster

This comment has been minimized.

Collaborator

stereobooster commented Jan 13, 2018

Or we can convert pagerror to event stream and convert crawling process to stream, merge both streams, reduce streams to promise, as soon as one of the streams fail we will reject Promise.

@peterbe

This comment has been minimized.

Owner

peterbe commented Jan 16, 2018

Or we can convert pagerror to event stream and convert crawling process to stream, merge both streams, reduce streams to promise, as soon as one of the streams fail we will reject Promise.

Uhhh? I understand about 0.1% of that :)

@stereobooster

This comment has been minimized.

Collaborator

stereobooster commented Jan 16, 2018

This is because I overcomplicated it. Pushed new version. Now seems to work as expected

@peterbe

You're doing such excellent work! I do see some important stylistic points that we should address.

src/run.js Outdated
new Error(`${response.status} on ${pageUrl} (second time)`)
return (
!pageerrorCalled &&
reject(new Error(`${response.status} on ${pageUrl} (second time)`))

This comment has been minimized.

@peterbe

peterbe Jan 16, 2018

Owner

Can you move this, and the block on line 194-197 to a function? Perhaps something like:

if (!response.ok) {
  return rejectResponse(response, 'second time')
}

and then that function could be something like

const rejectResponse = (response, message) => {
  if (!pageerrorCalled) {
     // We have not previously errored out before, so do it for this bad response
    const error = new Error(`${response.status} on ${pageUrl} (${message})`)
    reject(error)
  }
}
src/run.js Outdated
@@ -177,7 +177,12 @@ const minimalcss = async options => {
})
await new Promise(async (resolve, reject) => {
page.on('pageerror', error => reject(error))
let pageerrorCalled = false

This comment has been minimized.

@peterbe

peterbe Jan 16, 2018

Owner

The name isn't great. The point of this variable is to avoid calling reject() or resolve() more than once, right? How about something like this:

// If anything goes wrong, for example a `pageerror` event or 
// a bad download request (e.g. !response.ok), then remember that 
// we have fulfilled the promise and don't want to call `reject` or `resolve`
// a second time.
let fulfilledPromise = false
@peterbe

This comment has been minimized.

Owner

peterbe commented Jan 17, 2018

That big PR has landed now. You said this PR can wait :)

I'm really looking forward to this PR!

@peterbe

This comment has been minimized.

Owner

peterbe commented Jan 18, 2018

One way of testing this is:

node --trace-warnings bin/minimalcss.js -d --verbose http://localhost:8080/404css.html

where that URL is the 404css.html from the examples folder.
What happens is that this line happens but the whole process isn't stop.

Eventually you get a strange error about TypeError: Cannot read property 'type' of undefined because this code assumes there was a successful download and parse (to AST) for every URL covered.

So that's a solid way of testing. It should bail and crash early as soon as a CSS download fails.

@stereobooster

This comment has been minimized.

Collaborator

stereobooster commented Jan 19, 2018

@peterbe updated

@peterbe peterbe merged commit f99d7bf into peterbe:master Jan 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@peterbe

This comment has been minimized.

Owner

peterbe commented Jan 19, 2018

It works great! Thank you!
This deserves a new minor release.

@stereobooster stereobooster deleted the stereobooster:error-handling branch Jan 19, 2018

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