Expose ConcurrencyMode property in CallBackBehaviorAttribute.

[imcarolwang]

For issue #1959.

@mconnew I've added the public property, it seems the feature should work as it's applied in the attribute implementation:

 void IEndpointBehavior.ApplyClientBehavior(ServiceEndpoint serviceEndpoint, ClientRuntime clientRuntime)
        {
           ...
            dispatchRuntime.ConcurrencyMode = _concurrencyMode;
           ...
        }

Regarding test scenario, I need your guidance on how to test it, what kind of combination for instance mode and concurrency mode would you suggest to use to test the feature? Thank you.

[mconnew]

To test it, you have a callback contract with an operation which takes a parameter which says how long to sleep before returning. You have a service method which defines the callback contract operation as Task based which allows you to make a second call before the first one has finished. In the callback method you increment (using Interlocked.Increment to prevent race conditions) a number then sleep (if callback is implemented async, using `await Task.Delay). After the sleep/delay you set a ManualResetEvent so the main test code can be woken up where it can check the number of callbacks called.
Service code:
Task t1 = callback.CallWithWaitAsync(4000);
Task t2 = callback.CallWithWaitAsync(500);
await Task.WhenAll(t1, t2);

Callback service code:
Interlocked.Increment(ref counter);
await Task.Delay(delayTime);
manualResetEvent.Set();

Test code:
manualResetEvent.WaitOne(20000); // Add check to see if it timed out or completed. Fail test if timed out
Assert.Equal(2, counter);

If the callback is concurrent, the second call will execute while the first call is still running. This means the second call's ManualResetEvent.Set will happen first and the counter will be 2. If they are not concurrent, then the first calls ManualResetEvent will execute first and counter value will be 1.

Re-entrant is really difficult to test. I'm inclined to deprecate it and maybe modify the implementation code to throw if Reentrant is specified. It doesn't play well with Task's. I'll discuss with @HongGit and let you know. For now just write tests for Single and Multiple.

[mconnew]

I don't see how removing the try block fixed this. I believe you just got lucky when you submitted it again. What's happening is the manual reset event is reset at the end of the method call which then returns to the service. That then makes a second call back to the client and the client is being closed at the same time as another call is coming in. There's a race condition and you happened to avoid it when the tests got ran again. What you need to do is save the task from channel.DoWork() (which needs renaming to DoWorkAsync as it's an async method) and await it before moving to cleanup. That way you you know both callbacks have completed and there will be no error.

[imcarolwang]

I was trying to code according to my understanding, but it doesn't seem to work as expected, here is the code snippet:

        channel = factory.CreateChannel();
        Task task = channel.DoWorkAsync();

        Assert.True(imp.MyManualResetEvent.WaitOne(20000));
        Assert.Equal(1, imp.Counter);

        await task;

the await statement didn't wait for completion of the two tasks defined in DoWorkAsync which is awaiting Task.WhenAll(t1,t2). Should it work like this?

[mconnew]

The call to DoWorkAsync kicks off two async callbacks on the service side. In the service code you save each of the tasks and then await Task.WhenAll which shouldn't complete until both of the callback tasks have completed. One the await Task.WhenAll completes, the service side DoWorkAsync completes and sends the result (which just says it finished without an exception) to the client. On the client side the await on channel.DoWorkAsync task will complete when the server returns that completed message to the client. At that point there should be no requests in progress initiated by either the server or the client. The await task statement should wait for completion of the two tasks defined in DoWorkAsync.
I'll pull down your changes and see if I can work out what's going on.

[imcarolwang]

At server side, the line below doesn't await but return immediately:
await Task.WhenAll(t1, t2);
I got some problem attaching the repro app here, it's attached in a new comment here.

[mconnew]

Make s_manualResetEvent an instance field or property. When you create an instance of the class CallbackHandler_ConcurrencyMode_Single and pass it to the InstanceContext constructor, keep a reference to it and then you can access the manual reset event via the instance reference rather than statically.

[imcarolwang]

DuplexSvcCallbackTest.zip

[mconnew]

@imcarolwang, that repro app was really useful. I missed an important detail in your PR. You set the operations to OneWay = true. There's a subtle difference between a void (or Task) returning two way operation and a one way operation. With a void two way operation, the request is sent and the client waits for a reply SOAP message. That SOAP message will generally have an empty body, but if the operation throws, then it could contain a SOAP Fault message. A void two way operation method call won't complete until that reply SOAP message comes back. With a one way operation, the message is sent and the only reply expected is an immediate transport layer ack (an empty 200 response for HTTP and a 1 byte ACK for NetTcp). What was happening is the call to the on the client side service completed immediately as it was OneWay. You wait for the ManualResetEvent to be set, then awaited the DoWorkAsync Task which had already completed even though the service call was still ongoing. Then when closing the channel, the second call arrived which caused the exception. Here are the changes I made to get the test working:

1. Remove the OneWay property on the OperationContract attributes on server side and client side.
2. Add a ServiceBehavior attribute on the service side service class and set ConcurrencyMode = ConcurrencyMode.Multiple. Without this, it won't allow multiple callback calls because if one of those calls then calls back in to the service, you can deadlock.

I verified the Single mode had a count of 1 and the Multiple mode had a count of 2.

[imcarolwang]

@mconnew Thank you for the explanation! I understand now and I tried the fix on the repro app, it works as expected!
I've updated test accordingly, all the checks passed this time.