kvij / scuttle

A wrapper for applications to help with running Istio Sidecars
MIT License
9 stars 4 forks source link

Node JS SIGURG issue #13

Closed vitalii-buchyn-exa closed 1 year ago

vitalii-buchyn-exa commented 1 year ago

Hello community,

We are experiencing an issue with node js requests are failing and not even getting processed most of the times when we use scuttle (we use this fork).

From node js doc we know it uses that signal https://nodejs.org/api/os.html

We do see in logs scuttle ignoring it:

scuttle: Received signal 'urgent I/O condition', ignoring

We do see a patch for that case https://github.com/redboxllc/scuttle/pull/46.

But in our case we really need that signal to be bypassed to a child process.

Is it possible to add an argument or env variable or anything to mitigate that issue, please?

Thank you!

kvij commented 1 year ago

Hi Vitalii,

Doing that will send a lot of noise signals to node. I think you should get the pid of the child process and send the signals there instead of pid 1. Let me know if that does the trick. If not we can set up a test with a env flag. I'm on a holiday with limited internet access though so I can implement stuff after two weeks. On Mon, 28 Aug 2023, 09:53 vitalii-buchyn-exa, @.***> wrote:

Hello community,

We are experiencing an issue with node js requests are failing and not even getting processed most of the times when we use scuttle.

From node js doc we know it uses that signal https://nodejs.org/api/os.html

We do see in logs scuttle ignoring it:

scuttle: Received signal 'urgent I/O condition', ignoring

We do see a patch for that case redboxllc#46 https://github.com/redboxllc/scuttle/pull/46.

But in our case we really need that signal to be bypassed to a child process.

Is it possible to add an argument or env variable or anything to mitigate that issue, please?

Thank you!

— Reply to this email directly, view it on GitHub https://github.com/kvij/scuttle/issues/13, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEXLPSJZ2U3GZKLOV6QF4TTXXRE6BANCNFSM6AAAAAA4BAMWE4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

vitalii-buchyn-exa commented 1 year ago

Hi kvij,

Thank you for the reply! I'm not sure how to make node to behave like that... We don't send any signals from js app itself.

vitalii-buchyn-exa commented 1 year ago

Hi kvij,

I hope your holiday is going well.

I'm not allowed to push a brach to create a PR, so plz take a look into the following patch when and if you will have a time: 0001-allow-to-bypass-SIGURG-signal-with-env-var.patch

Thank you!

vitalii-buchyn-exa commented 1 year ago

just a note: when we switched back to envoy-preflight the issue is gone

kvij commented 1 year ago

Hi Vitallii,

I'm back :raised_hands:. As soon as I read that you don't send the signal itself I find it a very weird case. Are you sure you are not just seeing the log notification and correlate that to an issue? NodeJS is not even designed to run as PID 1 so I doubt that it is sending the signals to it. That would mean that on a full blown OS it would send signals to the init system.

I find the option you suggested rather dangerous. You can of course easily compile your own version and test if it truly fixes the problem or causes more. It should cause more problems because the Go runtime will generate SIGURG signals causing unnecessary priority socket checks in NodeJS. The normal workflow for contributing on github is forking the repo, make the changes on your own repo and then make a PR. It will automatically create a PR against the parent repo!

Hope this helps further investigating the issue. Let me know if there is something else I can help you with.

vitalii-buchyn-exa commented 1 year ago

Hi kvij,

Glad you back :)

Thank you for your suggestions, a PR created, i will test it and back to you with results. It shouldn't add any issue, because with envoy-preflight it works just fine and if i read it right, envoy-prefilight doesn't filter signals https://github.com/monzo/envoy-preflight/blob/master/main.go#L43 Anyways, there is only a correlation, not a direct evidence.

Regards, Vitalii

vitalii-buchyn-exa commented 1 year ago

Hello kvij,

Sorry for the huge delay with a response here. At the end, we cannot reproduce the issue with or without my patch. I think that was a bad coincidence. I also can't find that istio-proxy can send tcp traffic with URG flag enabled. Closing the PR and issue.

Thanks for your time and support.