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

Thread safe invocations #238

Closed
TKul6 opened this issue Mar 19, 2017 · 7 comments
Closed

Thread safe invocations #238

TKul6 opened this issue Mar 19, 2017 · 7 comments

Comments

@TKul6
Copy link

TKul6 commented Mar 19, 2017

Hello,

I wanted to create an "async shell" for methods to run asynchronously.

I created an AsyncInterceptor which look like this:

public void Intercept(IInvocation invocation)
        {


            //Methods starts with add_ or remove_ are registration and unregistration methods to the class's events.
            //Those methods should be invoked synchronously
            if (invocation.Method.Name.StartsWith("add_") || invocation.Method.Name.StartsWith("remove_"))
            {
                invocation.Proceed();
            }
            else
            {
                   Task.Run(() => invocation.Proceed());
            }


       }

Unfortunately, it didn't work. For some reason the interceptor was called much more than once.

By looking at AbstractInvocation I saw the Proceed method is not thread safe ( Mostly the usage of currentInterceptorIndex ).

I wonder if there is anyway to support this kind of behavior.

Thanks,

@osoykan
Copy link

osoykan commented Mar 19, 2017

There is a workaround solution that already implemented. It might give an idea.

@TKul6
Copy link
Author

TKul6 commented Mar 19, 2017

Unfortunately it won't work for me.
My intercepted methods return void, I just wan't to execute them on a different thread (it is a WCF callback contract and I want to free the WCF thread ASAP). My problem is the invocation.Proceed call itself being unsafe.
I know I can wrap it in a lock block and solve the issue, However I'll prefer if there is a built in solution for this type of behavior. like a built in thread safe invocation..

@ghost
Copy link

ghost commented Mar 29, 2017

@ghost
Copy link

ghost commented Mar 30, 2017

@TKul6
Copy link
Author

TKul6 commented Apr 3, 2017

Thanks for the replay, however I'm not sure It would help me. As I see it the problem lies within the Castle project. When I browsed the source code I saw the Proceed method of AbstractInvocation. inside this method the currentInterceptorIndex is increased \ decreased unsafely which causes race conditions and in my case invokes the same interceptor over and over again.

Is there any particular reason not handling the currentInterceptorIndex with Interlocked?

Thanks

@stakx
Copy link
Member

stakx commented Mar 23, 2019

Is there any particular reason not handling the currentInterceptorIndex with Interlocked?

Having recently reviewed #428 and #429, which also involve AbstractInvocation.currentInterceptorIndex, I don't think using Interlocked would fundamentally make a difference. It might solve this particular issue but there's the much larger problem that the increment/decrement dance has been written under the assumption that once an Intercept call returns, it is done intercepting. This premise no longer holds in today's TPL / async/await world where Intercept may set up continuations.

I think the solution should be to get rid of currentInterceptorIndex altogether. The big question is how to do this while minimizing the resulting breaking change.

@stakx
Copy link
Member

stakx commented Jul 14, 2019

I'm voting to close this issue. While AbstractInvocation still isn't thread-safe (due to currentInterceptorIndex), #439 now gives us a means to Proceed safely in async scenarios.

We should probably invest in thread safety some more for DP vNextMajor, but that is a different project from this issue (despite its title).

If noone disagrees, I'll close this in a few days' time.

@stakx stakx closed this as completed Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants