search

Danny-Dasilva / CycleTLS

Spoof TLS/JA3 fingerprints in GO and Javascript
GNU General Public License v3.0
941 stars 174 forks source link

Fix task killing by using built-in kill method #347

Closed marioparaschiv closed 3 months ago

marioparaschiv commented 4 months ago

By using the built-in method to kill a task, we guarantee a successful exit on all platforms for the child process.

Task killing on Windows currently errors when using CTRL + C to exit with:

ERROR: The process with PID 20108 (child process of PID 22720) could not be terminated. Reason: There is no running instance of the task.

marioparaschiv commented 4 months ago

The last change I have made works perfectly on Windows. I was only able to test it on Windows as I do not have access to a non-windows machine.

From my research, changing shell to false shouldn't affect anything on other platforms -- it just fixes the process termination issue on Windows.

Could someone could also test on Linux and Darwin?

marioparaschiv commented 4 months ago

Hey @Danny-Dasilva, could you have a look when you get the chance?

Danny-Dasilva commented 4 months ago

Testing this on Linux/Darwin

RealAlphabet commented 3 months ago

You can close https://github.com/Danny-Dasilva/CycleTLS/pull/344 in favour of this one. This PR fix the issue described in mine.

RealAlphabet commented 1 month ago

The last change I have made works perfectly on Windows. I was only able to test it on Windows as I do not have access to a non-windows machine.

From my research, changing shell to false shouldn't affect anything on other platforms -- it just fixes the process termination issue on Windows.

Could someone could also test on Linux and Darwin?

I encountered a problem with Node.js 20.X after upgrading, specifically, an ENOENT error. When I switched shell: false back to shell: true, the issue was resolved. This could potentially be an upstream bug with Node.js 20.X, as it seems not specifying shell: true might require manually handling environment variables such as PATH.

marioparaschiv commented 1 month ago

The last change I have made works perfectly on Windows. I was only able to test it on Windows as I do not have access to a non-windows machine. From my research, changing shell to false shouldn't affect anything on other platforms -- it just fixes the process termination issue on Windows. Could someone could also test on Linux and Darwin?

I encountered a problem with Node.js 20.X after upgrading, specifically, an ENOENT error. When I switched shell: false back to shell: true, the issue was resolved. This could potentially be an upstream bug with Node.js 20.X, as it seems not specifying shell: true might require manually handling environment variables such as PATH.

What shell are you using? I'm not sure how related it is but I thought in an attempt to try and reproduce your environment.

RealAlphabet commented 1 month ago

The last change I have made works perfectly on Windows. I was only able to test it on Windows as I do not have access to a non-windows machine. From my research, changing shell to false shouldn't affect anything on other platforms -- it just fixes the process termination issue on Windows. Could someone could also test on Linux and Darwin?

I encountered a problem with Node.js 20.X after upgrading, specifically, an ENOENT error. When I switched shell: false back to shell: true, the issue was resolved. This could potentially be an upstream bug with Node.js 20.X, as it seems not specifying shell: true might require manually handling environment variables such as PATH.

What shell are you using? I'm not sure how related it is but I thought in an attempt to try and reproduce your environment.

Sorry, I used strace to plot the difference between the two values and it turns out that the execve system call doesn't seem to appreciate hard links from pnpm which is my package manager. This has nothing to do with the version of NodeJS or the library