Panic when MaxRetry exceeded about fluent-logger-golang HOT 17 CLOSED

Hi all,

I was requested for advice by @ianlewis. After reading the code a bit I agree that panicking is not the best solution in this case. I would say that having a callback for errors is also not ideal.

Is there a reason why one would like the connection to stop reconnecting?
If not, I'd simply would remove the MaxRetry option.

If there's a valid reason for having this behavior (please let me know) I would probably handle it in one of these two ways:

func (f *Fluent) Status() Status

This could return some status indicating if the connection failed and how many times it did.
The downside of this is that it requires the client to be aware of this function and call it regularly to know decide what to do.

ConnectionErr

Have a specific error that is returned by any of the Post* methods. This error will contain the information about why the connection failed, how many times it's been retried, etc.

If MaxRetries is set the reconnect loop will simply stop trying, rather than panicking.

I'd go with the custom error. Does this sound reasonable?

from fluent-logger-golang.

IMO, panic should not use in library code basically.
But sometimes panic and recover are used together in library code to jump into top level.

So, we can choose two strategies to solve this issue.

1. Use panic and recover together to eliminate effect which causes aborting program. Here is minimal concept code for this situation: http://play.golang.org/p/5gQDHaA8iy
2. Change reconnect() function signature to return error information.

Is there anything else to solve this issue?

from fluent-logger-golang.

I set MaxRetry to -1 to avoid this panic temporarily...

from fluent-logger-golang.

Generally one would create an error channel or something to pass the errors back to the caller. However, I don't really see a way to fix the issue properly while maintaining backwards compatibility.

from fluent-logger-golang.

Any progresson this? It's affecting us as well.

from fluent-logger-golang.

Reported in moby/moby#32567

I think it makes sense to just have fluentd keep retrying with some max wait time.
I'd rather have logging block than panic the whole thing.

from fluent-logger-golang.

Changing default not to call panic makes the behavior of this library and programs which uses this (it may expect that users can find outage of destination by abnormal exit).
But there are no way to configure not to call panic - no way to help crashing whole process. It sounds not good.

So, we should:

• add a new option not to call panic (RetryForever ?) (default false)
• introduce a new configuration about MaxRetryWait (default value should be equal to retry wait of 13th retry)

Can anyone write patch for such behavior? I'm welcome for such change now.
(I'm busy now and cannot write that patch/tests by myself.)

from fluent-logger-golang.

@tagomoris It looks like we can get around the panic with a MaxRetry of -1? Would just need to make sure the retry wait doesn't grow forever. WDYT?

from fluent-logger-golang.

IMO using -1 as infinite is bad manner. It's not clear for all people, and I prefer to add much more explicit options.

from fluent-logger-golang.

I've sent a PR #48. How about this one?

from fluent-logger-golang.

@tagomoris How do you expect that programs are currently using this behavior? Currently the panic always happens in a goroutine created by fluent-logger-golang and so no one could possibly catch and handle the panic AFAICT. Are you saying they may expect that their whole program crashes when the fluentd server goes away? If we introduce a RetryForever, I see setting it to true as the only sane option for users.

I'm also concerned that there is no error logging or error channel that users listen to that is notified when there is failure connecting to the server. Currently if the fluentd server goes away, the client is essentially silent until the buffer fills up.

We may be able to learn from what the Google Cloud Logging client does when there is an error
https://github.com/GoogleCloudPlatform/google-cloud-go/blob/master/logging/logging.go#L99

from fluent-logger-golang.

@ianlewis Having callback functions for errors sounds good. But it's different thing from this issue. Of course, it's related with each other, but not directly.
Please open issue/p-r for it if you want to have it.

from fluent-logger-golang.

SGTM.

from fluent-logger-golang.

I just ran into the same issue and from the code and comments above, it looks like it has not been resolved yet. Any updates or any suggestions on how to deal with this, other than setting MaxRetry to -1?

from fluent-logger-golang.

I've implemented a fix for this in #57

from fluent-logger-golang.

I think this PR can be closed thanks to this commit... https://github.com/fluent/fluent-logger-golang/pull/56/files#diff-c6c15c7d149f5754061081471bfd402aL293

what do you think? @tagomoris

from fluent-logger-golang.

@kenju Thank you to notify this to me. Indeed.

from fluent-logger-golang.