electron-userland / electron-builder

A complete solution to package and build a ready for distribution Electron app with “auto update” support out of the box
https://www.electron.build
MIT License
13.46k stars 1.71k forks source link

fix: change abort to aborted event #8282

Open beyondkmp opened 4 days ago

beyondkmp commented 4 days ago

There are many SimpleURLLoaderWrapper server error in sentrty, I think they might be caused by this event.

Here's a simple example:

// test.js
'use strict';

// The same issue occurs with `http`, too.
const https = require('https');

const req = https.request('https://nodejs.org/dist/v13.7.0/node-v13.7.0.tar.gz');
req.on('response', (res) => {
  res
    .on('end', () => console.log('end'))
    .on('close', () => console.log('close'))
    .on('aborted', () => console.log('aborted'))
    .on('error', (err) => console.log(err));

  setTimeout(() => {
    console.log('start');
    req.abort();
  }, 500);
});
req.end();
changeset-bot[bot] commented 4 days ago

🦋 Changeset detected

Latest commit: 65d5bd067355ff715f1d59bbe81de1bfc716073b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ---------------- | ----- | | electron-updater | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify[bot] commented 4 days ago

Deploy Preview for car-park-attendant-cleat-11576 ready!

Name Link
Latest commit 65d5bd067355ff715f1d59bbe81de1bfc716073b
Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/668040c66b59ee0008dbbee4
Deploy Preview https://deploy-preview-8282--car-park-attendant-cleat-11576.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

beyondkmp commented 4 days ago

Here's a minimal example, with an 'aborted' event. However, I'm currently not sure why this event isn't documented.

// test.js
'use strict';

// The same issue occurs with `http`, too.
const https = require('https');

const req = https.request('https://nodejs.org/dist/v13.7.0/node-v13.7.0.tar.gz');
req.on('response', (res) => {
  res.on('aborted', () => console.log('aborted'))
  res.on('abort', () => console.log('abort'))

  setTimeout(() => {
    console.log('start');
    req.abort();
  }, 500);
});
req.end();
mmaietta commented 4 days ago

Okay, that's super weird indeed. Thanks for the test script! Maybe we should have both listeners (just in case for backward compatibility)?

Any chance you'd be willing to post a screenshot of what error you're seeing in Sentry?

beyondkmp commented 4 days ago

@mmaietta Here's the detailed sentry error. Because we recently enabled differential udpate, such errors have increased, so I suspect there is a connection here.

image

mmaietta commented 4 days ago

Any chance you'd be willing to test this on your app locally using patch-package first? I don't immediately see any harm/side-effects in the changes in this PR, but I don't have a way to verify locally via unit tests AFAICT.

beyondkmp commented 4 days ago

Unfortunately, I cannot reproduce this issue locally at the moment. It only seems to occur frequently in production when there is a large user base. I'll have to wait for production to test it out.