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

[Question]: Complex Pipeline Keys documentation example not working as expected #1926

Closed
jwagon opened this issue Jan 26, 2024 · 2 comments · Fixed by #1931
Closed

[Question]: Complex Pipeline Keys documentation example not working as expected #1926

jwagon opened this issue Jan 26, 2024 · 2 comments · Fixed by #1931
Labels

Comments

@jwagon
Copy link
Contributor

jwagon commented Jan 26, 2024

What are you wanting to achieve?

Hi,

I copied the code from from here:

https://www.pollydocs.org/advanced/dependency-injection.html#complex-pipeline-keys

Exactly into my project, but I kept getting "unable to find a resilience pipeline" exceptions.

What code or approach do you have so far?

When I looked through the code, specifically here:

if (_pipelines.TryGetValue(key, out pipeline))

public override bool TryGetPipeline(TKey key, [NotNullWhen(true)] out ResiliencePipeline? pipeline)
    {
        EnsureNotDisposed();

        if (_pipelines.TryGetValue(key, out pipeline))
        {
            return true;
        }

        if (_builders.TryGetValue(key, out var configure))
        {
            pipeline = GetOrAddPipeline(key, configure);
            return true;
        }

        pipeline = null;
        return false;
    }

It seems like if there is no pipeline defined with the key specified, like in your example:

 ResiliencePipeline instanceA = pipelineProvider.GetPipeline(new MyPipelineKey(ResiliencePipelines.NetSuite, "instance-A"));

It should go ahead and look in the _builders dictionary and build a new pipeline, with that new key, adding it to _pipelines, correct?

But I think (?) the problem is that in your example, PipelineNameComparer.GetHashCode takes the InstanceName into account:

public sealed class PipelineNameComparer : IEqualityComparer<MyPipelineKey>
{
    public bool Equals(MyPipelineKey x, MyPipelineKey y) => x.PipelineName == y.PipelineName;

    public int GetHashCode(MyPipelineKey obj) => (obj.PipelineName, obj.InstanceName).GetHashCode(); // <--- InstanceName is respected here, but maybe it shouldn't be?
}

I think (?) paying attention to InstanceName breaks your example, because when the key search in _builders (above) happens, it won't find any values.

Further, if I change that GetHashCode function to look like this, things work as I'd expect:

    public int GetHashCode(MyPipelineKey obj) => obj.PipelineName.GetHashCode();

I'm not sure what I might be breaking by making that change though, or if I'm just doing something else wrong somewhere.

Thoughts?

Additional context

No response

@jwagon jwagon changed the title [Question]: Complex Pipeline Keys not working as expected [Question]: Complex Pipeline Keys documentation example not working as expected Jan 26, 2024
@martintmk
Copy link
Contributor

@jwagon

You are correct in your investigation. The PipelineNameComparer should not include InstanceName. Fix is quite straightforward, I can do it over the weekend or you can open the PR yourself.

jwagon added a commit to jwagon/Polly that referenced this issue Jan 26, 2024
Addresses App-vNext#1926

In the Complex Pipeline Keys documenation example:

https://www.pollydocs.org/advanced/dependency-injection.html#complex-pipeline-keys

PipelineNameComparer.GetHashCode takes the InstanceName into account, but shouldn't. This fixes that line in the documentation.
jwagon added a commit to jwagon/Polly that referenced this issue Jan 26, 2024
Addresses App-vNext#1926

In the Complex Pipeline Keys documenation example:

https://www.pollydocs.org/advanced/dependency-injection.html#complex-pipeline-keys

PipelineNameComparer.GetHashCode takes the InstanceName into account, but shouldn't. This fixes that line in the documentation.
@jwagon
Copy link
Contributor Author

jwagon commented Jan 26, 2024

@jwagon

You are correct in your investigation. The PipelineNameComparer should not include InstanceName. Fix is quite straightforward, I can do it over the weekend or you can open the PR yourself.

@martintmk

PR opened, lemme know if I missed anything.

@martincostello martincostello linked a pull request Jan 26, 2024 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants