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

fix(deployer): added runtime error handling #1231

Merged
merged 5 commits into from
Oct 2, 2023

Conversation

iulianbarbu
Copy link
Contributor

@iulianbarbu iulianbarbu commented Sep 13, 2023

Description of change

  • Handled errors around runtime unexpected behavior by logging the errors and killing the runtime process.

When user services are CPU bound the runtime logs stream handled by the deployer can break with a broken pipe error (I haven't confirmed the root cause yet because it is not that straightforward - big stack based on tonic, hyper, and h2, but I expect it to happen because the runtime becomes CPU bound and it can not respond in time to the client keep-alive messages, which will be followed by closing the corresponding end of the tonic channel), exiting the run task with panic.

How has this been tested? (if applicable)

N/A

@iulianbarbu iulianbarbu changed the base branch from feat/shuttle-logger-service to main September 15, 2023 07:48
@iulianbarbu iulianbarbu force-pushed the fix/deployer-error-handling-2 branch 2 times, most recently from ecb9740 to 8de5040 Compare September 18, 2023 14:17
@iulianbarbu iulianbarbu force-pushed the fix/deployer-error-handling-2 branch from 6d091e7 to 6e2e703 Compare September 26, 2023 08:53
@iulianbarbu iulianbarbu force-pushed the fix/deployer-error-handling-2 branch from 6e2e703 to f9475aa Compare September 26, 2023 09:00
Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

Nice! We had a lot of unwraps here 😅

deployer/src/deployment/run.rs Outdated Show resolved Hide resolved
@oddgrd oddgrd merged commit b0d79a4 into shuttle-hq:main Oct 2, 2023
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.

4 participants