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

Missing IRecoveryable implementation #998

Closed
szarykott opened this issue Jan 8, 2021 · 4 comments
Closed

Missing IRecoveryable implementation #998

szarykott opened this issue Jan 8, 2021 · 4 comments

Comments

@szarykott
Copy link

szarykott commented Jan 8, 2021

I think I found one missing IRecoverable interface implementation.

Problem

As code snippet from master branch says in the comment it should be implemented for channels and connections, whereas it is only implemented for various classes related to autorecovering models.

namespace RabbitMQ.Client
{
    /// <summary>
    /// A marker interface for entities that are recoverable (currently connection or channel).
    /// </summary>
    public interface IRecoverable
    {
        event EventHandler<EventArgs> Recovery;
    }
}

What is more, actual event required by the interface is implemented for internal class AutorecoveringConnection (as RecoverySucceeded), just the class is not marked with IRecoverable interface implementation, only with IConnection. As class is internal, it seems not possible to even cast to it without reflection hacks to subscribe to the event.

It seems like a regression from previous versions as code suggests that it was meant to be implemented. Effectively it prevents my company from upgrading to newer version of library as we rely on Shutdown and RecoverySucceeded event to unpool faulty Rabbit instances.

We'd love to upgrade to newer version due to promised allocation improvements (and further ones in future versions as well!)

Expected behavior

Either:

  1. Implement IRecoverable interface for AutorecoveringConnection
  2. Remove comments and orphaned event handlers suggesting that this interface should be implemented (and enhance documentation regarding disconnection handling if possible)
@michaelklishin
Copy link
Member

You are welcome to submit a PR that restores the implementation of IRecoverable (arguably we should rename the event to RecoverySucceeded since Recovery is not specific at all).

@szarykott
Copy link
Author

Thank you for clarification! I will submit the PR soon.

@szarykott
Copy link
Author

I just noticed RecoverySucceeded is back in IConnection in version of the code currently on master. Having it there makes it not needed to implement what I proposed. Real question becomes - when a package version with it back will be published? I am using version 6.2.1 (from August 20th 2020) that seems newest one, while event was put back in IConnection mid September 2020.

@szarykott
Copy link
Author

Hey, any news on when could a new version of the library be released? Source code seems ok with what is currently on master, but there is still no new release.

Either way, it is possible that this issue could be closed as soon as the new version is released.

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

No branches or pull requests

3 participants