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

refactor(tm2/pkg/autofile): use SIGTERM #1006

Closed
wants to merge 1 commit into from

Conversation

ilgooz
Copy link
Contributor

@ilgooz ilgooz commented Jul 31, 2023

In order to make gnovm compatible with WASM target we need to use a signal other than SIGHUP. Here is the full list of support signals.

I changed the code to use SIGTERM instead, as it seemed closest to what SIGHUP does.

@ilgooz ilgooz requested review from jaekwon and moul as code owners July 31, 2023 08:22
@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Jul 31, 2023
Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

Changing SIGHUP to SIGTERM isn't ideal. SIGHUP is meant to be used multiple times in one process—it's a low-level signal for DevOps intent. SIGTERM just kills the process, making signal catching inefficient.

I'd say keep SIGHUP but disable the signal catching in an autofile_unsupported.go file where it's not supported (//+build wasm).


Also, not sure why SIGHUP just closes files without rotating logs. We should either scrap this or actually make it do a full log rotation. Thoughts, @jaekwon?

@ilgooz
Copy link
Contributor Author

ilgooz commented Jul 31, 2023

Thanks for the review. I will make the changes, but before moving forward, is there a possibility of autofile being refactored or a plan to change its behavior in a way that it won't depend on signals or SIGHUP? If so, we might just wait for that to come and close this PR entirely.

@moul
Copy link
Member

moul commented Jul 31, 2023

No refactoring plans yet, but I reckon we should reorganize the code (keeping the API as is). That'll let us have multiple implementations with build constraints and make wasm an official target for our packages.

Basically, it's about organizing our code files (in general) to make them easier to maintain, both for wasm and non-wasm.

As for completely axing signal handling or moving it to the main while letting the library follow the main configuration, that's probably the right path. But, I'm holding off for @jaekwon's take on my previous comment.

@ilgooz
Copy link
Contributor Author

ilgooz commented Aug 1, 2023

Closing in favor of #1012.

@ilgooz ilgooz closed this Aug 1, 2023
@ilgooz ilgooz deleted the ilgooz/refactor/autofile branch August 1, 2023 14:00
@moul moul added this to the 🌟 main.gno.land (wanted) milestone Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related
Projects
Status: Done
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants