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

Create a new process group on startup to allow safe shutdown when used with Ctrl-C console applications #262

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pablocrossa
Copy link

Issue:
Pressing 'Ctrl-C' to issue a console application termination on UNIX systems will send a SIGINT signal to all processes in the parent's process group.
When using the KPL, this means that the child C++ binary process will receive a SIGINT when pressing Ctrl-C. It will attempt to flush its internal buffers, but the libraries at the other side of the pipe that the programmer interacts with directly cannot identify all the final records as pushed or not.
This makes the KPL currently incompatible with applications that have a lifecycle that may include Ctrl-C to terminate.

Changes:
This PR includes a call to setpgid() on the child process on UNIX systems. This will create a new process group for the child process, which means it will not receive a SIGINT when the parent receives a Ctrl-C; it can still be send a SIGINT specifically with its PID, but it will not panic and flush when the parent receives Ctrl-C.
This change seems to have no adverse side effects on all the testing we have performed, the KPL still closes successfully on application shutdown and no zombie KPL processes are left over; this change should only affect the behaviour of the KPL when used in an application where the user presses Ctrl-C, and should stop the data loss experienced then.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

@ychunxue ychunxue left a comment

Choose a reason for hiding this comment

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

Because of the lack of knowledge for Native C, I am not able to verify if it would function as expected just by looking at the code. Can you please provide some details about how you tested it and how to verify if flush gets called when ctrl+c is pressed?

@micah-jaffe
Copy link

Thank you for submitting this pull request. We will triage this request to incorporate into future KPL releases. We don't have an ETA to provide at this time.

@mattjamison
Copy link

Would love to see this in an upcoming release. Without this change we lose records on shutdown in our apps attempting to use the KPL...

@yatins47 yatins47 closed this Nov 24, 2020
@yatins47 yatins47 deleted the branch awslabs:master November 24, 2020 15:20
@yatins47 yatins47 reopened this Nov 25, 2020
@mathiaspetersepi
Copy link

Hi,

we are using version 0.15.10 and are still experiencing the problem of uncontrolled shutdown when receiving a SIGINT. Do you plan on merging this PR anytime soon? This would be really helpful when using this lib in envs like Kubernetes or Docker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants