zeroc-ice / ice-demos

Sample programs for Ice
https://zeroc.com
GNU General Public License v2.0
323 stars 217 forks source link

Do not dispose CTS until communicator is disposed #192

Closed pepone closed 1 year ago

pepone commented 1 year ago

We should dispose the CTS only after the communicator is disposed of, otherwise request cancelation doesn't work. The CTS will no throw OCE after it is disposed.

bernardnormier commented 1 year ago

I don't understand the issue this PR is trying to fix.

We dispose the CTS before the communicator is destroyed and that's totally fine.

otherwise request cancelation doesn't work.

I am not following.

pepone commented 1 year ago

With the current code

jose@ubuntu22:~/3.7.9/ice-demos/csharp/Ice/async$ dotnet client.dll
usage:
i: send immediate greeting
d: send delayed greeting
s: shutdown server
x: exit
?: help

==> d
==> s
==> 

Here s invokes shutdown, which in turn cancels the token, and the cancelation triggers its disposal. After the token is disposed it no longer cancels the Task.Delay as you can see from the output the request isn't canceled.

bernardnormier commented 1 year ago

I think the issue is after:

cts.Token.WaitHandle.WaitOne();

returns, we dispose the cts then destroy the communicator (in this order), there is a small chance some dispatches are still running and didn't check cts.Token yet.

I think a better fix would be to wait for these dispatches to complete, with:

// existing comment
cts.Token.WaitHandle.WaitOne();

// Wait for all dispatches to complete before disposing cts 
// since the dispatches use cts or cts.Token
communicator.shutdown();
communicator.waitForShutdown();
pepone commented 1 year ago

I think the issue is after

reworked as suggested.