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

Renderer Node fs API stops working on page reload when allowRendererProcessReuse is true #22119

Closed
3 tasks done
lishid opened this issue Feb 10, 2020 · 43 comments · Fixed by #25869
Closed
3 tasks done

Comments

@lishid
Copy link
Contributor

lishid commented Feb 10, 2020

Preflight Checklist

  • I have read the Contributing Guidelines for this project.
  • I agree to follow the Code of Conduct that this project adheres to.
  • I have searched the issue tracker for an issue that matches the one I want to file, without success.

Issue Details

  • Electron Version:
    • 8.0.0
  • Operating System:
    • Windows 10 (1809)
  • Last Known Working Electron version:
    • Unknown

When setting app.allowRendererProcessReuse = true and after reloading the renderer (Ctrl+R in developer console), Node fs APIs would randomly stop working. Specifically, the callbacks are never called, or if you're using fs.promises, the promises are never resolved.

An interesting observation is: when you reload again, for a brief moment before the page is refreshed, the previous fs calls are magically unblocked. This can be observed by console.log lines suddenly printing the moment you hit Ctrl+R which disappears quickly as the whole page reloads.

Possibly related: #19554 This issue sounds like the opposite of what's happening here.

Also note: issue does not present itself when app.allowRendererProcessReuse = false

Expected Behavior

From the repro steps, expecting to see local files printed in the console:

...
renderer.js:8 Reading file main.js
renderer.js:8 Reading file package-lock.json
renderer.js:8 Reading file package.json
renderer.js:8 Reading file preload.js
renderer.js:8 Reading file README.md
renderer.js:8 Reading file renderer.js
renderer.js:14 done

Actual Behavior

List is printed the first time the app loads. Hit Ctrl+R and observe only a few lines printed, or none at all. Hit Ctrl+R again and observe more lines printed for a split second before renderer reloads.

To Reproduce

Clone https://github.com/electron/electron-quick-start

rendereer.js

(async () => {
	const fs = require('fs').promises;

	for (let i = 0; i < 10; i++) {
		let list = await fs.readdir('.', {withFileTypes: true});
		for (let file of list) {
			if (file.isFile()) {
				console.log('Reading file', file.name);
				await fs.readFile(file.name, 'utf-8');
			}
		}
	}

	console.log('done');
})();

main.js

const {app, BrowserWindow} = require('electron');

function createWindow() {
  const win = new BrowserWindow({
    width: 800,
    height: 600,
    webPreferences: {
      nodeIntegration: true
    }
  });

  win.loadFile('index.html');
  win.webContents.openDevTools();
}

app.whenReady().then(createWindow);

app.allowRendererProcessReuse = true;
app.on('window-all-closed', () => {
  if (process.platform !== 'darwin') {
    app.quit();
  }
});

app.on('activate', () => {
  if (BrowserWindow.getAllWindows().length === 0) {
    createWindow();
  }
});

Screenshots

First boot:
image
On reload:
image

Additional Information

@nathonius
Copy link

nathonius commented Mar 2, 2020

In my own code, if I have app.allowRendererProcessReuse = true;, I see the behavior described here. If I have app.allowRendererProcessReuse = false; I see the behavior described in #19554, though using setTimeout/setImmediate to call loadURL does not solve the issue.

Swapping to readdirSync and statSync seems to solve the issue, regardless of app.allowRendererProcessReuse setting.

On Win10, electron 8.0.2. Node 12.3.1.

EDIT: Upon further testing, using any promise based fs method locks the renderer. It also looks like electron 8.x uses Node 12.13.0; once I'm able I'll test with a more up-to-date version of node.

@Seblor
Copy link

Seblor commented Mar 5, 2020

I recently started updating all my project's dependencies, and I found the same behavior.

When updating to electron 6.0.0 or newer, setting allowRendererProcessReuse at true breaks many calls to fs (e.g. readDir, access, mkdir, exists) after a reload : sync methods hang, and callbacks never resolve.

However, the issue #19554 never happens to me (my config is based on Electron-Vue: https://github.com/SimulatedGREG/electron-vue/blob/master/template/src/main/index.js)

Environment: Electron 6+, Windows 10 (1903), Node 12.16.1 (LTS)

@nathonius
Copy link

I'm probably going to move the stuff I was using fs for to the main process anyway, but is there any reason this shouldn't work? Maybe some handles aren't getting closed?

lukechu10 added a commit to lukechu10/Minecraft-Box-Launcher that referenced this issue Mar 27, 2020
This fixes this issue: electron/electron#22119

This commit will be reverted once the issue on electron is fixed.
lukechu10 added a commit to lukechu10/Minecraft-Box-Launcher that referenced this issue Mar 28, 2020
This fixes this issue: electron/electron#22119

This commit will be reverted once the issue on electron is fixed.
@lishid
Copy link
Contributor Author

lishid commented May 19, 2020

With the official release of v9.0.0, app.allowRendererProcessReuse is now set to true by default.
Curious to know if the electron team is confident this issue isn't going to happen frequently?

I have the feeling that with the default changed, we can anticipate a lot of electron apps run into this weird bug that would cause the developers hours of headache on why their fs promises/callbacks never resolve. I have personally spent over 5 hours before concluding the issue was allowRendererProcessReuse and coming up with the repro steps.

So... just want to give it a bump. I think it's fair to be low priority while allowRendererProcessReuse was still off by default, but we can anticipate more trouble coming with the v9.0.0 release.

Thanks again for all this amazing work!

@obsius
Copy link

obsius commented May 19, 2020

I just upgrade to 9.0.0 and encountered this issue. In my case a call to fs worked, but a call to zlib hung indefinitely, and when I did a page refresh in dev tools the call returned, but then React error statements flooded my console:

React error caught in App: Error: An error was thrown inside one of your components, but React doesn't know what it was. This is likely due to browser flakiness.

At this point the page was locked up and I had to terminate the process.

I couldn't get this to happen all the time, but was able to reproduce about half the time. When I set app.allowRendererProcessReuse = false; I cannot reproduce this issue at all.

I know I'm not providing enough info to properly debug this issue, just adding my experience to the thread.

@JontyMC
Copy link

JontyMC commented Jun 4, 2020

With the official release of v9.0.0, app.allowRendererProcessReuse is now set to true by default.
Curious to know if the electron team is confident this issue isn't going to happen frequently?

I have the feeling that with the default changed, we can anticipate a lot of electron apps run into this weird bug that would cause the developers hours of headache on why their fs promises/callbacks never resolve. I have personally spent over 5 hours before concluding the issue was allowRendererProcessReuse and coming up with the repro steps.

So... just want to give it a bump. I think it's fair to be low priority while allowRendererProcessReuse was still off by default, but we can anticipate more trouble coming with the v9.0.0 release.

Thanks again for all this amazing work!

Agree with this, just spent a while trying to diagnose this issue.

@Hectate
Copy link

Hectate commented Jun 8, 2020

I'm fairly confident that I'm banging my head up against this one also. I've got chokidar watching a directory in the renderer and any amount of refreshing the page locks up the renderer. All the the rest of the UI and tools work, but the page itself stops working (a Vue instance) and responding to input.

@lishid
Copy link
Contributor Author

lishid commented Jun 8, 2020

@sofianguy any chance this issue can get a new look in terms of prioritization? I'm specifically referring to my previous comment here to justify #22119 (comment)

Also happens on Electron 9.x.y

@afloras
Copy link

afloras commented Jun 12, 2020

Funny, i had this same issue yesterday #24073 and it was closed prematuraly ;-;
The only choice seems to set allowRendererProcessReuse to false if you use electron 9.0 or higher for now, but what will happen if this property gets deleted in the future? Doc says :

The intention is for these overrides to become disabled by default and then at some point in the future this property will be removed. (https://www.electronjs.org/docs/api/app)

For me, i think the callbacks are called to the previous renderer, so if i reload a page or go to another one the page is strangely still connected to the previous renderer process. I mean, there's no error, everything is ignored in the callback function, so i don't see any other explanation than that... right?

@mahhov
Copy link

mahhov commented Jul 8, 2020

Just updated from electron 8 to 9. Spent 2 days trying to figure out why my https requests (using node's https module) would never return after refreshing from the dev console. Surprisingly, doing some hacky setInterval( /* make a new https request to the same endpoit */ , 500) seemed to "wake up" the pending requests, but I never got this ironed out. Disabling allowRendererProcessReuse got the http requests working again.

I see electron 11 plans to Remove the ability to change app.allowRendererProcessReuse in Electron 11. I hope this is resolved prior to disabling this workaround.

@nathonius
Copy link

I think there has to be some other underlying issue, otherwise this would be more widespread. I'm just not sure what causes this condition.

@pushkin-
Copy link

pushkin- commented Jul 8, 2020

not exactly this issue, but a related one here

@aabuhijleh
Copy link

Still reproducible in the latest Electron versions #24173

@compeek
Copy link

compeek commented Aug 6, 2020

I just ran into what I believe is this same issue, but not with the Node API. I have an Angular 10 app running, and after upgrading to Electron 9, I found that after refreshing, the Developer Tools showed a message saying it was disconnected, and my page would be pretty much blank. Refreshing again would cause the page to appear somewhat, but all of my database queries (Knex with sqlite3) would hang.

I determined Electron 9 was the issue and then guessed and found that setting allowRendererProcessReuse to false fixed it.

It's possible under the hood there are the Node API is being used, I suppose, but for me it was the promises from Knex never resolving.

@remss
Copy link

remss commented Sep 29, 2020

@codebytere, I still see the issue in the release 10.1.3

@codebytere
Copy link
Member

@remss do you have a reliable repro? The one in this issue no longer occurs for me 🤔

@pushkin-
Copy link

@codebytere I can repro the dupe closed issue. Pasting repro below:

main.js:

(async () => {
     const { BrowserWindow, ipcMain, app } = require("electron");

     app.allowRendererProcessReuse = true; // Workaround 1: comment this out
     await app.whenReady();

     const mainWindow = new BrowserWindow({
          show: true,
          webPreferences: {
                nodeIntegration: true,
               enableRemoteModule: true
         }
     });

     mainWindow.loadFile("./firstPage.html"); // Workaround 2: load the secondPage instead of the firstPage

     ipcMain.once("next", () => {
          mainWindow.loadFile("./secondPage.html");
          mainWindow.webContents.openDevTools();
     });
})();

firstPage.html:

<!DOCTYPE html>
<html>

<body>
     <button id="acceptButton">Accept</button>
     <script src="./firstPage.js"></script>
</body>

</html>

firstPage.js:

document.getElementById("acceptButton").onclick = () => {
     require("electron").ipcRenderer.send("next");
     // Workaround 3: Do a process.exit() here and await the loadFile(secondPage) navigation. This causes us to create a new renderer process
};

secondPage.html:

<!DOCTYPE html>
<html>

<body>
     <div id="controlDiv">
          <input id="url" type="url" value="https://google.com" />
          <button id="navigateButton">Navigate</button>
     </div>

     <script src="./secondPage.js"></script>
</body>

</html>

secondPage.js:

const https = require("https");
const http = require("http");
const dialog = require("electron").remote.dialog;

document.getElementById("navigateButton").onclick = () => {
     const url = (document.getElementById("url")).value.trim();

     const parsedURL = new URL(url);
     let requestMethod = http.request;
     if (parsedURL.protocol === "https:") {
          requestMethod = https.request;
     }

     const clientRequest = requestMethod(url, { method: "HEAD", timeout: 3000 }, () => {
          dialog.showMessageBox({ message: "valid" });
     });

     clientRequest.on("timeout", () => {
          dialog.showMessageBox({ message: "timed out" });
          clientRequest.abort();
     });

     clientRequest.end();
};
  1. npm start
  2. click Accept button
  3. click Navigate
  4. note, first navigate may show a valid popup, but all subsequent navigations fail with a "timed out" error dialog

@KishanBagaria
Copy link
Contributor

I can sadly confirm the dupe #25311 isn't fixed in 10.1.3 either (don't have a reproducible gist.)

@codebytere
Copy link
Member

Thanks! Will keep digging

@Seblor
Copy link

Seblor commented Sep 30, 2020

I can confirm the issue is not resolved with 10.1.3. I managed to reproduce the issue with those steps:

  1. Clone the Electron Quickstart repo and install dependencies
  2. Enable nodeIntegration
  3. Write the following code in the renderer.js file:
const fs = require("fs");

let counter = 0

const counterInterval = setInterval(() => {
  document.body.innerText = `Attempting to read README.md file... (${++counter}s)`
}, 1000);

fs.readFile("README.md", (err, data) => {
  if (err) {
    document.body.innerHTML = `An error occured: ${err}`
    return
  }
  document.body.innerHTML = `Got content of README.md:<br><pre>${data.toString()}</pre>`
  clearInterval(counterInterval)
})

Now when starting the app, the content of README.md will be displayed the first time, but reloading (CTRL + R or "View" > "Reload") will make the fs call hang.

@codebytere
Copy link
Member

codebytere commented Oct 9, 2020

On which operating system @Seblor? I ran https://gist.github.com/f56fde13e73569b3b9f5b7cc6edde1b7 and still could not reproduce on macOS.

@Seblor
Copy link

Seblor commented Oct 9, 2020

@codebytere I used your exact files and still have the error when reloading the app.

I ran it on Windows 10 (2004), Node 13.8.0 and Electron 10.1.3.

Tell me if you need more info, and thanks for looking into this issue.

@codebytere
Copy link
Member

codebytere commented Oct 9, 2020

@Seblor success! i was on macOS - it seems now to be more specific to Windows.

Additionally, this seems more specific to the async call - if i swap readFile for readFileSync I don't see this issue

@codebytere
Copy link
Member

As an update for all who run into this - we recognize this bug is a blocker for many and do not plan to remove allowRendererProcessReuse until it is resolved.

@codebytere
Copy link
Member

Fix is up at #25869, folks!

@KishanBagaria
Copy link
Contributor

@codebytere is it Windows-only based on the title or all platforms? 🤞

@codebytere
Copy link
Member

codebytere commented Oct 12, 2020

@KishanBagaria the issue was most prominent on Windows but this fix should ameliorate issues cross-platform! I'll update the title actually.

@ffflorian
Copy link
Contributor

Looking forward to testing it. Thanks already for working on it @codebytere!

@ffflorian
Copy link
Contributor

ffflorian commented Oct 26, 2020

@codebytere unfortunately the problem still exists in Electron 10.1.4.

Update: it was a problem with my VM. Now it works, thanks @codebytere!

@KishanBagaria
Copy link
Contributor

Haven't seen this issue occur since Electron 10.1.4. Definitely has reduced if anything.

@lishid
Copy link
Contributor Author

lishid commented Dec 27, 2020

@codebytere After many many sessions of debugging I found a repro (I'm on Windows 10):

  • Perform a long-running async node call (in my case, the call was fs.writeFile)
  • Reload the page before the call finishes.
  • Observe that any subsequent node calls never triggers the callback.
  • Reload again fixes the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
8.2.x
Unsorted Issues
Development

Successfully merging a pull request may close this issue.