electron-builder icon indicating copy to clipboard operation
electron-builder copied to clipboard

fix: change abort to aborted event

Open beyondkmp opened this issue 1 year ago • 7 comments

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();

beyondkmp avatar Jun 29 '24 08:06 beyondkmp

🦋 Changeset detected

Latest commit: 3cb844685e10563a07a64f77b343bcc4d94d1bc5

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

changeset-bot[bot] avatar Jun 29 '24 08:06 changeset-bot[bot]

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

Name Link
Latest commit 3cb844685e10563a07a64f77b343bcc4d94d1bc5
Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/66968621d2a89000080b74e9
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.

netlify[bot] avatar Jun 29 '24 08:06 netlify[bot]

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();

beyondkmp avatar Jun 29 '24 17:06 beyondkmp

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?

mmaietta avatar Jun 29 '24 17:06 mmaietta

@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

beyondkmp avatar Jun 29 '24 17:06 beyondkmp

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.

mmaietta avatar Jun 29 '24 17:06 mmaietta

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.

beyondkmp avatar Jun 29 '24 17:06 beyondkmp

@beyondkmp any luck with your patched production build rollout? Looking to determine next steps with this PR 🙂

mmaietta avatar Jul 15 '24 17:07 mmaietta

It hasn't been released yet. Once it is released and we have results, I will provide immediate feedback.

beyondkmp avatar Jul 16 '24 04:07 beyondkmp

@mmaietta Based on the release results, network errors have not increased; instead, they have decreased, although they have not been completely eliminated.

From the results, my suggestion is that this can be merged.

beyondkmp avatar Aug 09 '24 06:08 beyondkmp