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

Multiple ways to short-circuit pipeline is a poor design and should be reconsidered #54548

Closed
PonchoPowers opened this issue Mar 15, 2024 · 3 comments
Labels
area-hosting Includes Hosting design-proposal This issue represents a design proposal for a different issue, linked in the description ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. question Status: Resolved

Comments

@PonchoPowers
Copy link
Contributor

Summary

Having the ability to call Run on the pipeline is confusing because you can have multiple calls to it and get no error, and the last element in the pipeline is effectively a Run anyway. Run has no logical difference to Use if the middleware in the Use call short-circuits. I propose removing Run because it can cause Use calls to be skipped and there is no error to inform the programmer of this issue.

Motivation and goals

  • Simplify the framework by reducing the number of methods that can be called
  • Remove ambiguity as to which should be used
  • Avoid silent pitfalls

Risks / unknowns

The risk is that is a breaking change, some might be using the Run call.

Examples

There are two ways to short-circuit:

By not calling next.Invoke()...

app.Use(async (context, next) =>
{
    // Do something
    // Don't call await next.Invoke();
});

And by calling app.Run instead of app.Use like so...

app.Run(async (context) =>
{
    // Do something
});

Run is signalling to the programmer that no more pipeline elements will execute, whereas app.Use may or may not short-circuit. If Run is intended, why not just order the pipeline correctly so that the last item in the chain is the last item? The naming of Run is also ambiguous with the later call to Run which looks visually unappealing as if it were a bug when it isn't.

@PonchoPowers PonchoPowers added the design-proposal This issue represents a design proposal for a different issue, linked in the description label Mar 15, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Mar 15, 2024
@javiercn javiercn added area-hosting Includes Hosting and removed area-blazor Includes: Blazor, Razor Components labels Mar 17, 2024
@javiercn
Copy link
Member

@BonnieSoftware thanks for contacting us.

This is not a direction we plan to take. It's a huge breaking change and we haven't received significant feedback about it to justify it, and even so, we would prioritize backwards compatibility over "poor design" (a value judgement that we and others don't necessarily share).

@javiercn javiercn added question ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. labels Mar 17, 2024
@PonchoPowers
Copy link
Contributor Author

If people don't necessarily share the same view, what view do they share, how is app.Run of a good design, what use is it?

@PonchoPowers
Copy link
Contributor Author

PonchoPowers commented Mar 18, 2024

I feel quite strongly about this, the issue should be reopened, in all the codebases in Github, the only time it is ever used is in tests relating to itself, the odd bit of debugging, and examples showing it can be used. In all of these cases app.Use could have simply been used. Github has 28 million public repositories. Therefore, how big of a breaking change is this really?

It particularly looks good when used like so, as the following isn't at all confusing...

app.Run();
app.Run();

Also, could you please answer my earlier question about what use it is, if even possible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-hosting Includes Hosting design-proposal This issue represents a design proposal for a different issue, linked in the description ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. question Status: Resolved
Projects
None yet
Development

No branches or pull requests

2 participants