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

Fix async problem calling await before Proceed - Breaking Change #428

Closed

Conversation

brunoblank
Copy link

This PR solved the async problem discussed in #145

Breaking Change:
void IInvocation.Proceed() is moved to void IInterceptor.Intercept(IInvocation invocation, InvocationDelegate proceed);

Before:

public class Interceptor : IInterceptor
{
   public void Intercept(IInvocation invocation)
   {
       invocation.Proceed();
   }
}

Becomes:

public class Interceptor : IInterceptor
{
   public void Intercept(IInvocation invocation, InvocationDelegate proceed)
   {
       proceed(invocation);
   }
}

stakx added a commit to stakx/Castle.Core that referenced this pull request Mar 21, 2019
This is a variant of castleproject#428. Unlike that earlier PR, this take does not
build a complete chain of `proceed` delegates at the beginning of in-
terception. Instead, it abstracts the progress of an interception as a
cheap-to-build, immutable value type that now also hosts the `Proceed`
action (which previously sat on `IInvocation`). Functionally, this
should be very similar to castleproject#428, but without most of the GC pressure.
Copy link
Member

@stakx stakx left a comment

Choose a reason for hiding this comment

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

First of all, a big thank you for contributing this idea. 👍

While the basic idea behind this PR appears sound to me, I am very hesitant of such a big breaking change to one of DynamicProxy's main APIs. I'd like to withhold any definite judgement on this PR for the time being, but still like to provide some feedback on it.

Personally, I think that moving Proceed out of IInvocation might be a good thing, as it arguably improves the abstraction of the latter: IInvocation would then (more than now) describe an invocation without tying it to the interception pipeline that processes it. This would arguably fit its name better. So far, so good.

To me, the big con of this PR is that it potentially adds a lot of unnecessary overhead: You're building a full chain of proceed delegates at the very beginning of every single invocation, regardless of whether they will ever get called or not. Being reference-typed objects, these delegates will be allocated on the heap and add to GC pressure. I feel that this overhead is not acceptable in a library that supposedly generates "light-weight" proxies.

However, this problem can be solved. Based on your PR, I've prepared one possible solution; see stakx@9f67f16 (part of my fork's async-interception branch). It builds the next Proceed step on demand only, taking advantage of cheap-to-build value types and in parameters to avoid unnecessary copying. Terminology might not be ideal, but if you like the general idea, feel free to incorporate or adapt it in your own work.

As I said, I am not yet decided whether such a breaking change is worth it, but there's probably no harm in toying with these ideas in the meantime.

@stakx
Copy link
Member

stakx commented Mar 21, 2019

@brunoblank - In case that we do decide to take this further, I think it would be great if you could cherry-pick or otherwise incorporate @JSkimming's unit test from #429 into your PR.

@stakx
Copy link
Member

stakx commented Mar 29, 2019

#439 just got merged, so I am closing this PR as it was meant to enable the same use case.

Thank you @brunoblank for having figured out how to solve this long-standing problem with Proceed! 🙇‍♂️

@stakx stakx closed this Mar 29, 2019
@brunoblank
Copy link
Author

Awesome! Glad I could help out finding a solution for the annoying async problem.

/Bruno

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

Successfully merging this pull request may close these issues.

None yet

2 participants