search

treasure-data / serverengine

A framework to implement robust multiprocess servers like Unicorn
Apache License 2.0
759 stars 86 forks source link

Add an option to disable Sigdump #93

Closed gmitrev closed 6 years ago

gmitrev commented 6 years ago

As reported in https://github.com/treasure-data/serverengine/issues/92, the Sigdump.dump command on receiving a SIGCONT causes lots of unwanted files in systems where systemd is used to control services that use serverengine.

This PR adds a new setting called disable_sigdump that can be used to disable this behaviour. It is false by default since we don't want to break anything for current users.

robinbowes commented 6 years ago

As commented (but not quickly enough!) on #92...

I would suggest that changing the signal used to trigger the dump would be the best option: https://github.com/treasure-data/serverengine/blob/678bb269ab13e62277d28981b0c7047b146e72dd/lib/serverengine/signals.rb#L29

Perhaps SIGUSR1 ?

robinbowes commented 6 years ago

I don't know context but I guess changing this is only an issue if this forms part of a published API and if we have users that expect SIGCONT to trigger a coredump.

gmitrev commented 6 years ago

@robinbowes I didn't want to change the behaviour for existing users hence this PR. A change of the signal is an official API change and would be more appropriate for a larger release.

tagomoris commented 6 years ago

Oops. CI on Windows is failing.

gmitrev commented 6 years ago

@tagomoris Any idea how to debug this? I see that the the log is incomplete and the runtime of each test is 1hr which probably means it timed out. Previous builds took around a minute. Is there a way to retrigger the build?

gmitrev commented 6 years ago

@repeatedly @tagomoris The CI is passing, although it's quite erratic and tests sometimes fail because of random system slowdowns. Do you need me to do anything else to move this forward?

repeatedly commented 6 years ago

I see. Code itself looks good.

@tagomoris how about this?

tagomoris commented 6 years ago

LGTM. Unstable CI is another problem anyway... @gmitrev Nice patch!

mitio commented 6 years ago

Hey guys, can we merge this?

tagomoris commented 6 years ago

Oh, I missed to do it. @mitio Thank you!