AtomLinter / linter-eslint-node

ESLint plugin for Atom/Pulsar Linter (v8 and above)
https://web.pulsar-edit.dev/packages/linter-eslint-node
MIT License
4 stars 3 forks source link

Don't prevent worker exit on `unhandledException` #37

Closed savetheclocktower closed 1 year ago

savetheclocktower commented 1 year ago

This is the fix to #16 that I described in this comment.

I've aimed to make the package more vocal in its logging when things go wrong, and made it so that you don't have to be in dev mode just to see the logs — there's an advanced option to enable logging.

UziTech commented 1 year ago

Someone is working on a setup-pulsar action to replace setup-atom. It would be great to get the tests working with pulsar.

savetheclocktower commented 1 year ago

Doesn't action-pulsar-dependency work as a replacement for action-setup-atom?

UziTech commented 1 year ago

I haven't really looked at it but I don't think you can specify a version of pulsar to test.

UziTech commented 1 year ago

Also it looks like that only works on linux

savetheclocktower commented 1 year ago

Cool. Yeah, that'd be great. Also @Spiker985 pointed me to their workflow for x-terminal-reloaded that does automatic publishing to the Pulsar package repo, so maybe that'll be useful as well.

I can help with devops stuff if it's truly necessary, but it would really stretch the definition of the word “help.”

Spiker985 commented 1 year ago

action-pulsar-dependency installs Pulsar in the same manner a user would for their respective OS

With the caveat that it automatically appends pulsar to the PATH on Windows - allowing you to call pulsar on any platform without having to specify an explicit path

And you can then use something like coactions/setup-xvfb to run pulsar --test <spec>

And the version of Pulsar it downloads, is the most recent passing rolling release, as that is our primary release strategy

UziTech commented 1 year ago

And the version of Pulsar it downloads, is the most recent passing rolling release, as that is our primary release strategy

Right so no way to test on beta and stable versions or have a minimum supported version tested.

UziTech commented 1 year ago

I can help with devops stuff if it's truly necessary.

I wouldn't say it is necessary but right now the tests are not doing much since they could be failing in pulsar and we wouldn't know.

savetheclocktower commented 1 year ago

Right, and I did run them locally. I figured that it was better to fix this bug than it was to leave it unfixed for lack of cross-platform tests. I might be able to work on it once I finish up the main Pulsar project I'm working on right now.

UziTech commented 1 year ago

It would be nice if you could create a test to make sure this doesn't happen in the future and to make sure this actually fixes the problem.

savetheclocktower commented 1 year ago

I agree that it would be nice, and I'd be thrilled if I could get it to reproduce consistently in the first place. This scenario was so hard for me to trigger that, when it did happen, I dropped everything I was doing so that I could attach a debugger and get to the bottom of it. I found the symptom that was causing it to loop endlessly, but not the root cause.

I am honestly open to any suggestions on how I can repro this, given that the instructions in #16 didn't ever work for me. Failing that, the best I can do is make this change so that it's clear that the uncaughtException block simply cannot get itself into an endless loop.

savetheclocktower commented 1 year ago

OK, here's how I finally repro'd it: I made the worker wait 5 seconds before it returned linting results, and then I reloaded my Pulsar project while I was waiting for linting results. That was enough to put the worker in an endless loop, and to make me realize that JobManager wasn't actually killing the worker when it got disposed.

I've added a new test; it's a bit silly, but it wouldn't pass against master, so it's got at least some value. Now we've got a belt-and-suspenders approach to the problem and a regression test. I remain unsure how to test the 100% CPU utilization properly, but this'll surely be better than nothing.