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

feat: remove panics from applications #5943

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

SWvheerden
Copy link
Collaborator

Description

Applications currently panic on any thread panic. Remove this so that applications will not panic.

Motivation and Context

See: #5940

This is somewhat debatable if this is desired or not behaviour. But currently, the favoured approach is to only panic the thread.

Fixes: #5940

Copy link

Test Results (CI)

1 257 tests   1 257 ✔️  23m 35s ⏱️
     39 suites         0 💤
       1 files           0

Results for commit bc45e24.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Nov 10, 2023
Copy link

Test Results (Integration tests)

  2 files  11 suites   16m 18s ⏱️
31 tests 30 ✔️ 0 💤 1
32 runs  31 ✔️ 0 💤 1

For more details on these failures, see this check.

Results for commit bc45e24.

Copy link
Contributor

@brianp brianp left a comment

Choose a reason for hiding this comment

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

LGTM.

As per the removed comment. Does this mean if some service does Panic the app may risk silently continuing while that particular service of the app has died?

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Nov 10, 2023
@SWvheerden
Copy link
Collaborator Author

Yip, not my preferred choice, I prefer global panic. But it seems if its not the preferred approach.

@SWvheerden SWvheerden merged commit 18c3d0b into tari-project:development Nov 10, 2023
12 of 13 checks passed
@brianp
Copy link
Contributor

brianp commented Nov 10, 2023

Maybe we'll have to investigate some kind of timed-out respawn of lost services

@SWvheerden SWvheerden deleted the sw_remove_panic branch November 14, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attackers can crash whole node by panicking a single thread
3 participants