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

Operation not permitted #103

Closed
ericdude4 opened this issue Feb 22, 2020 · 41 comments
Closed

Operation not permitted #103

ericdude4 opened this issue Feb 22, 2020 · 41 comments

Comments

@ericdude4
Copy link

Got the following error from Sentry produced by our production app. Could this be due to some odd permissions that the user has changed on their system?

OS: Windows 10.0.17763
Electron: 8.0.0
electron-store: 3.3.0

Error: EPERM: operation not permitted, rename 'C:\Users\mattd\AppData\Roaming\clockk\config.json.2393936311' -> 'C:\Users\mattd\AppData\Roaming\clockk\config.json'
  at Function.writeFileSync [as sync](app:///node_modules/write-file-atomic/index.js:198:8)
  at ElectronStore.set store [as store](app:///node_modules/conf/index.js:267:19)
  at ElectronStore.set(app:///node_modules/conf/index.js:152:14)
  at _.clearArtifacts(app://./js/app.1a7ce22d.js:1:20601)
  at ? (app://./js/chunk-vendors.3f748be6.js:38:5706)
  at ? (app://./js/chunk-vendors.3f748be6.js:38:6751)
  at Array.forEach(<anonymous>)
  at ? (app://./js/chunk-vendors.3f748be6.js:38:6730)
  at _._withCommit(app://./js/chunk-vendors.3f748be6.js:38:8276)
  at _.commit(app://./js/chunk-vendors.3f748be6.js:38:6704)
  at _.commit [as originalCommit](app://./js/chunk-vendors.3f748be6.js:38:3374)
  at EventEmitter.<anonymous>(app://./js/chunk-vendors.3f748be6.js:85:28802)
@sindresorhus
Copy link
Owner

This might be a case of npm/write-file-atomic#28.

@RoderickQiu
Copy link

This might be a case of npm/write-file-atomic#28.

I'm currently having this problem too... The bug doens't always show, but it do occurs from time to time though. According to the issue you mentioned, the problem seems already solved, but it seems that this problem is still with this module with the newest 5.1.x series.

@RoderickQiu
Copy link

Would the problem vanishes if we could disable all the errors thrown? Though it doesn't seems very safe and dev-friendly.
Anyway, if electron-store supports syntax of promise, I think things can be better as this problem is likely to be caused by writing one files at the same time by 2 or more processes.

@RoderickQiu
Copy link

RoderickQiu commented Feb 26, 2020

store.set("last-recorded-state", {
       workTime: workTime / 60000,
       restTime: restTime / 60000,
       loop: loop,
       title: title,
       note: note,
       isWorkTimeFocused: store.get("fullscreen-work"),
       isRestTimeFocused: store.get("fullscreen")
});
store.set("last-recorded-method", methodFromStart);

The problem occurred again. This is the code that caused bugs this time.

Or, should I just discard this kind of code? If so, what is better? Thanks.

@lilis7575
Copy link

I am having the same problem.

This problem occurs intermittently.
And, everything happens in window7.

OS: Windows 7 sp1
Electron: 7.0.0
electron-store: 5.1.0

@RoderickQiu
Copy link

I am having the same problem.

This problem occurs intermittently.
And, everything happens in window7.

OS: Windows 7 sp1
Electron: 7.0.0
electron-store: 5.1.0

Seems that it's a hard-to-solve question... I suggest using trycatches for backporting or prevent 2 processes from modifying the stored content at the same time though it's a bit of uncomfortable to do so and could make your code not so strong...

@chaveezy
Copy link

chaveezy commented Apr 1, 2020

Also experiencing this same thing, I initially thought it was due to an antivirus issue, but I realized that I am calling two store.set() at the same time. I then added a setTimeout(function(){store.set()},1000); and I thought I had it fixed but I still get the same issue. the timeout was to try and allow for the first store.set() to complete. Maybe this was the wrong approach?

@RoderickQiu
Copy link

image

I've once again read the doc of this proj, and this time I found something may be good for you (and for me, too). Haven't tested myself, though I'm thinking that by using this, your code may need to be complex.

Anyway, I think for using this lib, try-catches are necessary (and it also seems to be the most convenient and least costly solution to partially fix this?) to make your code stronger...

@mifi
Copy link

mifi commented Apr 16, 2020

A user of my app also just reported the same error mifi/lossless-cut#312

I don’t know what’s causing it. I asked the users some questions about what he was doing. Maybe we need to solve it the same way as the EXDEV error by catching and trying again with a normal write?

@mifi
Copy link

mifi commented Apr 17, 2020

I've been doing some research. I think it boils down to the fact that Windows API for rename is very unstable and depends on a lot of factors like anti-virus, any open file handles, and the moon phase.

electron-store is using write-file-atomic, which uses the plain Node.js fs api. I think for this to be improved, write-file-atomic need to instead use fs-extra (which handles the EXDEV issue #106) and it uses graceful-fs which handles the EPERM issue with a 1 second retry loop.

One problem is that even with graceful-fs there are still issues with EPERM on windows even though it has 1 second retry logic

And I don't think the write-file-atomic guys want to use introduce two extra layers of abstraction by adding fs-extra dependency, because write-file-atomic is being used by a lot of packages including npm and it might introduce regressions for a lot of people.

So I think either electron-store needs to either:

  1. Change the code that uses write-file-sync to instead write the file directly with no atomicity guarantee. (or only do so if platform.os === 'win32')
  2. include EPERM to the catch block if-check along with EXDEV to retry with fs.writeFileSync. I'm not sure if this will work because EPERM seems to be caused by the target file being occupied by antivirus or something else for some time, and I'm not sure if writeFileSync will also throw EPERM if the file is still occupied
  3. Write our own write-file-atomic implementation replacement that does the same but instead uses fs-extra which better handles these errors.
  4. Find another way to guarantee atomicity when writing a config file
  5. Keep it as is, but give the developers huge warnings in the electron-store README about the fact that the set and new Store() functions is likely to fail often on Windows and that they need to handle this in their code with try/catch around these functions

@mifi
Copy link

mifi commented Apr 17, 2020

Actually as of now, the developer needs to also wrap the the call to const store = new Store(); with try/catch, because the constructor runs set which in turn runs _write

@sindresorhus
Copy link
Owner

  1. Even if only on Windows. This is not a great idea. Before we had atomic writing, we got reports from user having their config corrupted.

I'm fine with either 2 or 3, but I would prefer 3. write-file-atomic has consistently been badly maintained and badly coded. I welcome a fork that's better maintained.

@fabiospampinato
Copy link

fabiospampinato commented Jul 4, 2020

I welcome a fork that's better maintained.

@sindresorhus Here we go (hopefully): https://github.com/fabiospampinato/atomically

I'm basically on your same boat and I couldn't find anything better than write-file-atomic, so I rewrote it. I'd say atomically is probably strictly better than write-file-atomic, it handles more errors (including the one mentioned here), it can be 10x faster on synchronous writes and just as safe (I've no idea why in my benchmark it isn't also 10x faster in asynchronous writes), it has way more options (including a way to hide temporary files) etc.

On the code quality I can't comment too much being the author of this one, but at least when putting the async and sync version of the provided functions side by side the implementation is basically identical, in write-file-atomic there are a bunch of quirks because this isn't true there too. Unfortunately the test suite is still pretty messy as I inherited the one from write-file-atomic, but throwing their tests away didn't feel right and rewriting the entire test suite didn't feel worth it.

Happy to work on any eventual issue you guys can find, I'm going to use this library in an Electron app too so any issue you can find I'll be interested in fixing it.

@jurepetrovic
Copy link

Ok, so we also noticed these problems. I followed @fabiospampinato's advice and integrated atomically into
electron-store. Now you have an electron-store fork, which uses atomically inside. We started using it and so far no problems on win machines. If anybody needs this, please use it and let me know how it goes. I would really appreciate feedback. Especially from some heavy used electron apps!

https://github.com/jurepetrovic/electron-store-atomically

Thanks,
Jure

@sindresorhus
Copy link
Owner

PR welcome to switch to this library.

@ericdude4
Copy link
Author

@jurepetrovic please make a PR, I've been hoping for this fix for months :p

@jurepetrovic
Copy link

I have made PR #130 , but it requires some more testing for real production.
Actually the real change is in conf-atomically, where the atomically is actually used.

I had to change some project data, since I published separate npm package on npmjs for our own use.

Thanks and Rgds,
Jure

@jurepetrovic
Copy link

https://www.npmjs.com/package/electron-store-atomically

If we merge this successfully and it works under windows, I'd be very happy to use this original package!

@sindresorhus
Copy link
Owner

Released: https://github.com/sindresorhus/electron-store/releases/tag/v6.0.0

@lights0123
Copy link

lights0123 commented Jul 20, 2020

Side note: this increases the minimum supported Node.js version to 12.12.0. However, electron 9 ships with Node.js 10. This seems like a problem...?

@fabiospampinato
Copy link

fabiospampinato commented Jul 20, 2020

@lights0123 Electron v9.1.0 ships with v12.14.1. Actually you highlighted a bit of an issue with atomically, the actual supported version number might be even v8, I didn't set it properly to begin with when I was using fs.promises, and now the library isn't actually even using that, and I forgot to update the minimum required node version. PR welcome to set the correct one 👍

@lights0123
Copy link

Yep, I was thinking this issue was about Node.js 11 so I was thinking one less.

@bad2Dbone
Copy link

User told me, that when application starts from .exe file, this error occurs. If the app is started from console, no problem appeared until now.

Maybe this information could be useful to somebody.

@fabiospampinato
Copy link

@bad2Dbone which version of the library are you using? The one with write-file-atomic or the one with atomically?

@bad2Dbone
Copy link

currently the template given for new version: "electron-store-atomically": "^0.0.2",
but Ill try electron-store 6.0.0 and let you know...

@fabiospampinato
Copy link

fabiospampinato commented Jul 22, 2020

@bad2Dbone It sounds like you are using a version with atomically already, in that case there isn't too much that can be done, like there are no guarantees that write operations will always succeed, especially in hostile environments like Windows with an aggressive antivirus, you might want to consider switching to asynchronous writes rather than synchronous, as the former have a higher default timeout, or you could just provide atomically a higher up timeout is that sounds like a good trade off to you, although currently I don't think conf and electron-store allow you to change these things 🤔

@jurepetrovic
Copy link

Ok, so we know 2 things:

  • We could try with higher timeout values in atomically. Will try to follow up on that.

  • Switch to async solution - however conf does not support this, and we would have to do something on our own completely? In this case electron-store cannot be used.

Am I uderstanding this correctly?

@fabiospampinato
Copy link

@jurepetrovic Yeah. Switching to async reads and writes is more about them having a higher default timeout in atomically really, and you don't always want that, like waiting 5 seconds for a write to succeed at startup or something might not be worth it. There's also this that could help, I haven't implemented it yet though.

@jurepetrovic
Copy link

There is another observation which seems important.

  • In development environment, where we use "yarn start" that starts development server and loads localhost:3000 into electron this problem does not happen.

  • In packaged environment, where we load static files the problem happens. Any idea what could here be the difference between these two? In both cases, the loaded code is executed in electron's context. See pic.

image

All ideas seriously appreciated ! (beer)

@fabiospampinato
Copy link

fabiospampinato commented Jul 23, 2020

@jurepetrovic There are all sorts of factors that have an impact on this, including if the sun is shining outside or not, and we can't really do much about them, maybe in your case just having all files packages in an asar archive makes the antivirus take longer to scan it and unlock it, even if the part of the archive being scanned is not the one you currently care about. But I don't think you are writing into the asar so probably that doesn't matter 🤔

@jurepetrovic
Copy link

Ok :) We'll try with this timeout first and then let you know here ;-)

Well - despite everything that is going on here and the number of libs included - I still believe computers are exact
science. So there has to be a reason for everything.

@ericdude4
Copy link
Author

@sindresorhus Can we reopen this issue? I'm still getting the same error reports from client machines.

@bad2Dbone
Copy link

We ar now using https://github.com/jurepetrovic/electron-store-atomically which has higher timeout for writing. There were no problems detected so far.

@ericdude4
Copy link
Author

@bad2Dbone Is it a drop-in replacement for electron-store?

@jurepetrovic
Copy link

Yes, you can replace it as-is. But only thing changed is the timeout (from 100ms to 500ms) that is given to the operating system for finishing the write operation. In other words, we allow the operating system more time to do the write operation before aborting.

@FE-linmu
Copy link

FE-linmu commented Apr 8, 2021

I still have this problem

electron-store:6.0.1
electron:8.5.2

@FE-linmu
Copy link

FE-linmu commented Apr 8, 2021

@sindresorhus Can we reopen this issue? I'm still getting the same error reports from client machines.

Does my brother still have this problem?

@fabiospampinato
Copy link

IMHO pretty much the only lever that can be pulled is giving atomically more time to write stuff by raising the timeout and/or switching to async writing. If there's anything else that can be tried to fix this I'm no aware of this.

@gaetandezeiraud
Copy link

I have this problem in production. Not everytime, but often enough to be disturbing.
electron-store: 8.0.1
electron: 8.0.1

@PDKSophia
Copy link

I have the same problem ...
electron-store: 8.1.0
electron: 22.0.1

@liees
Copy link

liees commented Mar 27, 2023

I have the same problem ... electron-store: 8.1.0 electron: 22.0.1

这个问题你已经解决了嘛?

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

No branches or pull requests