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

Issues with IClusterClient #4436

Closed
dandago opened this Issue Apr 9, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@dandago
Contributor

dandago commented Apr 9, 2018

  1. Should we call Close() or Dispose() to clean up the client, or both?
  2. IClusterClient has an awaitable Connect() method, whereas ISiloHost has an awaitable StartAsync(). That -Async() suffix is inconsistent. I know Microsoft recommends the -Async() suffix, but the Orleans team had decided against it a while back. Either way, it should be consistent. Personally, I prefer the -Async() suffix as it is immediately recognisable as needing an await (or other Task-based approach).
@sergeybykov

This comment has been minimized.

Member

sergeybykov commented Apr 9, 2018

We inherited StartAsync() from aspnet/Hosting#1163 via #3431. Hence "Async" in the method name. Otherwise, we try not to add the suffix.

I personally don't see a clean and easy way out of this compromise. It's too late for us to rename public methods anyway.

@sergeybykov sergeybykov added this to the Triage milestone Apr 9, 2018

@ReubenBond

This comment has been minimized.

Contributor

ReubenBond commented Jun 21, 2018

@dandago Close() is graceful. Dispose() is identical to Abort(), which is ungraceful. So, if possible, always opt for await client.Close();

@ReubenBond ReubenBond self-assigned this Jun 25, 2018

@ReubenBond

This comment has been minimized.

Contributor

ReubenBond commented Jun 25, 2018

Does this answer your question, @dandago?

@dandago

This comment has been minimized.

Contributor

dandago commented Jun 26, 2018

Yes, although I'm very surprised at this design decision. This goes against a common convention in the .NET Framework of Close() and Dispose() being equivalent, and prevents people from wrapping their client in a using block.

@ReubenBond

This comment has been minimized.

Contributor

ReubenBond commented Jun 26, 2018

@dandago the client is intended to be long-lived, unlike a DbContext, which may be per-request. So the client typically lives for the duration of the app.

Close() is not identical to Dispose() because it's naturally async.

Here's the relationship between them:

                    graceful
           Close() +------------+
              (async)           |
                                |
                    ungraceful  v
           Abort() +----------> Stop(bool gracefully) (private)
           ^  (blocking)          * if graceful, shutdown gracefully
           |                      * clean up internal state
Dispose() ++                    
@dandago

This comment has been minimized.

Contributor

dandago commented Jun 26, 2018

Why does Close() need to be async in this case? If it's doing cleanup, that will typically involve internal resources and no I/O operations.

@ReubenBond

This comment has been minimized.

Contributor

ReubenBond commented Jun 26, 2018

@dandago because it tells the cluster that the client is disconnecting, because it's graceful.

@dandago

This comment has been minimized.

Contributor

dandago commented Jun 26, 2018

OK, got it.

@dandago dandago closed this Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment