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

Detect and handle connection lost errors #50

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

e-nastasia
Copy link

@e-nastasia e-nastasia commented Dec 26, 2023

Hello @johnmanjiro13!

Thank you for the work you've put into creating this crate! I am happy it exists and I find it very useful.

I am having one issue with it though: when my binary loses connection to fluent, there's no way for me to catch this situation and handle it because the connection errors aren't propagated from the crate.

So I've created this pull request to address the issue: with these changes crate users will be able to get access to the connection lost errors so they can do something about it.

@e-nastasia e-nastasia marked this pull request as ready for review January 9, 2024 22:15
Copy link
Owner

@johnmanjiro13 johnmanjiro13 left a comment

Choose a reason for hiding this comment

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

Hi, @e-nastasia. I'm sorry for delay.
Thank you for pointing it out.

Could you check some comments?

Err(_) => continue,
Err(e) => match e {
Error::MaxRetriesExceeded => {
error!("Reached MaxRetriesExceeded");
Copy link
Owner

Choose a reason for hiding this comment

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

Could you use lowercase for error messages?
ref: rust-lang/api-guidelines#79

break;
}
Error::ConnectionClosed => {
error!("Reached ConnectionClosed");
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can't access this error because it isn't returned to the main task.
Are you assuming that this pull request is a first step to fix a connection lost problem?

@johnmanjiro13 johnmanjiro13 added the bug Something isn't working label Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants