search

FWeinb / electron-screenshot-service

Take screenshots using electron
MIT License
141 stars 26 forks source link

Fix browsercount #21

Closed jerbob92 closed 8 years ago

jerbob92 commented 8 years ago

The browsercount in closeAll is incorrect.

FWeinb commented 8 years ago

Thanks for doing this PR. Totally missed that one. Is this ready or are you still working on it?

jerbob92 commented 8 years ago

Ah sorry. I forgot I made a PR for this. I continued to work on this as this just won't work for us in the current form.

The only relevant change is "this.browserCount = 0;" > "this.count = 0;"

However, I also found out that browsers are not actually closing and thus leaving leftover processes.

FWeinb commented 8 years ago

On what OS are you experiencing these issues. I tested it on macOS and it worked fine in the past.

jerbob92 commented 8 years ago

On Linux (Ubuntu inside a docker container), when I run app.quit(), the actual Electron process never gets closed. When I run app.exit(0) it does work. I had some major problems with hanging/crashing electrons so now I'm just calling screenshot.close(); (which triggers browser.closeAll();) before I take a screenshot so I always have a fresh browser process. It's a little slower but at least I know I have a working service.

I run 4 docker containers with a renderer to scale.

jerbob92 commented 8 years ago

I created a proper PR in #22