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

fix #121 #159

Merged
merged 4 commits into from
Mar 19, 2019
Merged

fix #121 #159

merged 4 commits into from
Mar 19, 2019

Conversation

silkentrance
Copy link
Collaborator

@silkentrance silkentrance commented Nov 30, 2017

  • interactive apps running on windows require more work for SIGINT as it does not support signal
    and sending it a CTRL-C signal requires a console on Windoze, which is not what tmp is about
    we might add additional tests for this, but, frankly, I am sick of the Windoze platform and will not be
    implementing any more work arounds or even dedicated tests for it
  • SIGTERM cannot be handled in any way, so the temporary objects will remain
  • SIGINT is now handled correctly even with jest sandboxing in place (OSX/Linux only, on Windoze, the tests will be skipped)
  • for this to work, we also need to drop support for node 6 as it will not work on Linux, whereas it will work just fine on OSX for that same version, it seems as if signal handling on node 6/Linux is not working as one would expect it to

raszi
raszi previously requested changes Dec 1, 2017
test/child-process.js Outdated Show resolved Hide resolved
test/child-process.js Outdated Show resolved Hide resolved
test/spawn-custom.js Outdated Show resolved Hide resolved
test/spawn.js Outdated Show resolved Hide resolved
test/outband/issue121.js Outdated Show resolved Hide resolved
test/issue121-test.js Show resolved Hide resolved
test/issue121-test.js Outdated Show resolved Hide resolved
lib/tmp.js Outdated Show resolved Hide resolved
lib/tmp.js Outdated Show resolved Hide resolved
lib/tmp.js Outdated Show resolved Hide resolved
@silkentrance
Copy link
Collaborator Author

@raszi we definitely need linting rules in place, so you do not have to this 😁

@raszi
Copy link
Owner

raszi commented Dec 2, 2017

We do have linting rules. We did not have a task for it, but here is a PR for that in #163.

@silkentrance
Copy link
Collaborator Author

rebased to master

@silkentrance
Copy link
Collaborator Author

Tests require a lot more work. I am on it but it will take some time as I am currently working on multiple other things as well.

@silkentrance
Copy link
Collaborator Author

appveyor is broken, all tests have passed and still they report failing builds...

@silkentrance
Copy link
Collaborator Author

It seems as if there is some process still running, causing a timeout for the builds on appveyor.
Need to investigate this using a windows machine SIGH.

@silkentrance
Copy link
Collaborator Author

This seems to be a long standing issue

nodejs/node#12378

@silkentrance
Copy link
Collaborator Author

silkentrance commented Oct 6, 2018

Regardless of the fact that the tests are all off, not to mention that they are all shize. Need to fix these ASAP.

signal parameter is never passed, need to investitate why.

@silkentrance
Copy link
Collaborator Author

Heck, I need a windoze machine for this finally to be resolved.

@silkentrance
Copy link
Collaborator Author

silkentrance commented Mar 13, 2019

The fix does not work on windows. In fact, appveyor builds will timeout after 1hr. sigh more work to be done to fix this for this darn platform.

I need to be able to cancel build jobs on appveyor #185.

@silkentrance
Copy link
Collaborator Author

hell, even with the latest changes, the SIGINT listener will not be called on windows...

@silkentrance
Copy link
Collaborator Author

Signal handling on the Windoze platform is not an option. Support is available for OSX/Linux only, starting with node > 6. Node 6 on Linux has issues that will prevent the signal SIGINT listener from being called.

@silkentrance
Copy link
Collaborator Author

silkentrance commented Mar 16, 2019

I am giving up on making this work on Windoze using standard node provided IPC/process signalling.
The platform, while it claims to be compatible to POSIX standards, plainly sucks.
The associated tests will be skipped on that platform.

Copy link
Owner

@raszi raszi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, this is a nice PR @silkentrance, thank you very much!

My only problem is that this whole graceful cleanup was just a small feature of this library and now we are having hard time supporting it. I believe we should consider dropping it in the near future.

test/issue121-test.js Outdated Show resolved Hide resolved
@silkentrance
Copy link
Collaborator Author

First of all, this is a nice PR @silkentrance, thank you very much!

My only problem is that this whole graceful cleanup was just a small feature of this library and now we are having hard time supporting it. I believe we should consider dropping it in the near future.

You are welcome. Thank you very much.

Yeah, the graceful cleanup stuff is getting out of hand :)

And it also comes with the safe installation of the listeners, something that is required for this to work flawlessly with the jest testing framework.

I will look into this and see whether it can be simplified. Maybe we can externalise parts of this by exposing the garbage collector?

dropping support for node v6.x as it is not working correctly with the installed SIGINT handlers
add appveyor build for node 11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants