Worker died unexpectedly

[hugoduraes]

This issue started happenning today, after updating linter-eslint to version 5.1.0:

Console output:

Atom version: 1.2.0
Linter ESLint: 5.1.0

[steelbrain]

It's an electron bug, It has been fixed in the latest Atom beta. See electron/electron#3446

[rabbitfang]

Would it be possible to add a fix for the worker crashing? Maybe automatically restart the worker if needed? That way, it would fix the issue until Atom 1.3 comes out and in the future if a similar bug occurs.

[steelbrain]

@rabbitfang Restarting the worker could be a good workaround, can you open a new issue for it?

[EvHaus]

I'm also seeing this but my Worker Info object has this output:

stderr: "(8183,0x7fff7aa82300) malloc: *** error for object 0x7fe0f2f3afb0: pointer being freed was not allocated↵*** set a breakpoint in malloc_error_break to debug↵"
stdout: ""

Is this the same issue?

[steelbrain]

@globexdesigns Your output matches this issue exactly electron/electron#3446

[eyalw]

so when will the fix be released (by Atom)?

[Arcanemagus]

It is fixed in the v1.3.0 betas, please use that for now or wait until it gets released to stable.

[arnaud-dezandee]

I got bored of 100's of crash a day, and created another plugin with a different approach to using eslint pkg. Works well for me, maybe it will help others: fast-eslint

[Arcanemagus]

@Adezandee you could also have just updated Atom to the v1.3.0 beta which fixes the electron bug causing this... or was that not working for you?

[arnaud-dezandee]

@Arcanemagus sure, but I also felt that the plugin lacks some speed probably coming from all the main-child process async communication (which overheads a bit ~300ms, see Node.js docs)

[iam4x]

@Adezandee I'm actually doing the same, but instead I've installed an older version of this package. $ apm install linter-eslint@4.0.0 👍

I'm thinking about a fork, because I disagree on the recent changes and where this plugin is going now. And I've been working on AtomLinter since first closed beta of Atom.

[SpainTrain]

I implore folks to improve this project rather than creating multiple forks. When politics like these spawn multiple forks, it creates confusion among users and more redundant work for the maintainers of all the forks (lots of the same bugs, features, etc.). Please strongly consider working together to create (and support) something superior to what any of us would be able to create individually.

From what I have seen, anyone that participates by contributing code or discussion is taken seriously and their ideas are respected. @iam4x, IMO your ideas and opinions still carry by far the most weight because you created it ❤️ and have strong atom expertise.

Regarding this issue specifically, a ton of users were not able to use the linter at all (#210) and the only concrete solution brought forth to fix it was #258. There are still issues of course, but forward is better than stagnant, IMO.

Since this is way off topic, let's move any further discussion to the atom slack channel (#linter-eslint).

[Arcanemagus]

@Adezandee you are definitely correct that that works much faster, although I'm not sure the delay is due to the async communication (if so it's not 300ms minimum).

Here is some performance data I just took by putting a performance.now() at the start of the lint function, and ending the timer right before the mapping of the results in both packages. As you can see linter-eslint is much less consistent in it's timing, and ranges from an average of 4.7 to 5.7 slower.

The entire reason this was split out into a worker process is that it was blocking the rendering thread. Here is a CPU profile of a "hot" run from the file of the first data set in the timing spreadsheet. From what I can tell, your package is running on the rendering thread which isn't ideal, but I'm not saying splitting off an entire new process like is currently being done is the answer. Of the 367ms that the lint() from within linter was running fast-eslint accounted for 278 ms (1379.4 to 1630) of that in total, while linter-eslint only accounts for 1 ms (1630 to 1631 ms). Can we do something with worker threads? I'm not a JS expert here so I'm sure I'm only scratching at the surface of what is going on.

Why don't we work together on getting this returning results as fast as your version, but still keeping from blocking the main thread?

[arnaud-dezandee]

@Arcanemagus awesome data, thanks for your efforts. I am not a JS threads expert either. This is two different ways of doing things, I don't see any way of merging those two in a meaningful way.

@SpainTrain totally agreed, still linter-eslint chosen a very specific way (child process) of doing this. For me, that's not really a fork (and it's ~50 lines of code).

[SpainTrain]

There are projects that expose the web worker API in node to run code in multiple threads, could be promising:

• https://www.npmjs.com/package/webworker-threads
• https://www.npmjs.com/package/paralleljs

and one that has its own api

• https://www.npmjs.com/package/threads_a_gogo

[iam4x]

@SpainTrain Was thinking about that aswell. I'm giving it a go!

[Arcanemagus]

If we can get this working properly we can expand the idea to other linters, from the trace you can see linter-jscs blocked for 64 ms as well. (Even though it's set to onlyConfig and doesn't have a config for this project)

[Arcanemagus]

Seems like https://www.npmjs.com/package/webworker-threads is the one to go with, the other two haven't been updated in 2 years and it purports to comply with the WebWorker standard API.

[SpainTrain]

Liberal use of the term "fork". Two projects in the same ecosystem doing the same thing. But I get your point.

@Arcanemagus do you have the issue # handy for the perf issue that lead to the worker solution? Didn't find it earlier. Any solution that doesn't cause a regression on that ticket is fair game IMO.

[SpainTrain]

I was thinking of #195 maybe. Also, cannot have regression on #210 (I was never able to repro, so no clue how to test 😨)

[steelbrain]

@iam4x @Adezandee Let us not split this repo up into forks while you can improve this one. From what I've observed so far, IPC adds no more than 1-10 ms in a process, what's actually time consuming in this linter is all the syscalls it does, in find operation to be exact. There's a lot of low hanging fruit with a lot of benefit.

@Adezandee Your linter provider doesn't support all of the scenarios and configurations this linter does, therefore it's minimal and faster. But if you were to add any more feature in it, your users would experience a lag. I used to experience that lag of about 4 seconds each time with a newly started atom when I would lint a js file.

[pnkorn]

Atom 1.9.9 and

Uncaught (in promise) Error: Given editor is not really an editor(…)
set @   message-registry.js [sm]:41
(anonymous function)    @   linter.coffee:37
module.exports.Emitter.simpleDispatch   @   /Applications/Atom.app/Contents/Resources/app.asar/node_modules/event-kit/lib/emitter.js:25
module.exports.Emitter.emit @   /Applications/Atom.app/Contents/Resources/app.asar/node_modules/event-kit/lib/emitter.js:125
(anonymous function)    @   linter-registry.js [sm]:66