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

Replace Connection.Closed event with Connection.StateUpdated #892

Closed
bentoi opened this issue Mar 9, 2022 · 9 comments
Closed

Replace Connection.Closed event with Connection.StateUpdated #892

bentoi opened this issue Mar 9, 2022 · 9 comments
Labels
proposal Proposal for a new feature or significant update

Comments

@bentoi
Copy link
Contributor

bentoi commented Mar 9, 2022

A StateUpdated event would be more useful than the existing Closed event.

A question however is which state we should report to the applications.

A non-resumable connection goes through the following states: NotConnected -> Connecting -> Active -> ShuttingDown -> Closing -> Closed. The Closed state if final. It doesn't go through the ShuttingDown state if the application explicitly calls CloseAsync or if the connection is lost.

I think these states are fine to report through for the StateUpdated event of non-resumable connections.

Today, a resumable connection goes through the following states:

  • NotConnected -> Connecting -> Active -> Closing -> NotClosed if the connection is lost or if CloseAsync is called
  • NotConnected -> Connecting -> Active -> ShuttingDown -> Closing -> NotConnected if ShutdownAsync is called
  • NotConnected -> Connecting -> Active -> ShuttingDown -> Closing -> Closed if DisposeAsync is called.

But note that there's some discussions on #891 and in particular if this should instead be the following for resumable connections:

  • NotConnected -> Connecting -> Active -> Closing -> NotConnected if the connection is lost.
  • NotConnected -> Connecting -> Active -> ShuttingDown -> Closing -> Closed if ShutdownAsync or DisposeAsync is called. If CloseAsync is called, it doesn't go through the ShuttingDown state.

From an application perspective, I think the second option is better for resumable connections.

@bentoi bentoi added the proposal Proposal for a new feature or significant update label Mar 9, 2022
@ReeceHumphreys
Copy link
Contributor

This makes a lot more sense to me than the current way we are doing it!

@bernardnormier
Copy link
Member

I think:
NotConnected -> Connecting -> Active -> ShuttingDown -> Closing -> Closed

has too many states, and the names are not consistent. Take the first 3, it should be:
NotConnected -> Connecting -> Connected
or
Inactive -> Activating -> Active

not some mix.

And nobody will understand the distinction ShuttingDown / Closing.

@bernardnormier
Copy link
Member

Or a related note, I think we should move the event registration to ConnectionOptions and ServerOptions, and not expose the event in the Connection public API.

@bernardnormier
Copy link
Member

I think these states are fine to report through for the StateUpdated event of non-resumable connections.

It would be good to outline a use-case for each of these state transitions. If there is no use-case, we probably don't need the state.

For example, assuming NotConnected -> Connecting -> Connected, what's a use case for the first transition ("ConnectAsync was called on the connection but has not done anything yet").

My understanding is all these events execute synchronously and serially.

@bentoi
Copy link
Contributor Author

bentoi commented Mar 11, 2022

One use case is a GUI notification, such as client using a resumable connection where the NotConnected, Connecting and Connected states are useful to show the state of the connection.

I agree that ShuttingDown and Closing are dodgy from an application perspective. Ice doesn't have this distinction because we have an internal state enum and a simpler public state enum. However, in the early days of IceRpc, we changed it to keep a single enum (simpler from the implementation perspective).

To me, from an application perspective the following would be the simplest: NotConnected, Connecting, Connected, Closing, Closed. We could eventually merge NotConnected and Closed state and rename Closing to Disconnecting (use Disconnected instead of Closed)?

The state names are independent of the API method names however so it might be obvious when these state changes are triggered (knowing that they can be triggered explicitly through API calls or implicitly by the core).

@bernardnormier
Copy link
Member

I find having both NotConnected and Disconnected is a bit confusing.

The NotConnected state is very transient. Do we really need it? Can an application see a connection in the NotConnected state? If not, the initial state could be Connecting:

Connecting -> Connected -> Disconnecting -> Disconnected.

@bentoi
Copy link
Contributor Author

bentoi commented Mar 11, 2022

Today, it's not a transient state since a resumable connection always goes back to the NotConnected after being closed. A connection created by the application is also in the NotConnected state until the application either calls ConnectAsync or performs an invocation through the connection.

But yes, we could still remove it and instead create the connection with the Disconnected initial state.

@bernardnormier
Copy link
Member

I see the following stable connection states. "stable" meaning a connection can stay in that state for hours and all is fine.

  • Initial
  • Connected
  • Disconnected
  • Closed [terminal]

Only a resumable connection can be in the Disconnected state. The Disconnected state is very similar to the Initial state. The difference today is the Features of the Connection in the initial state is empty whereas in the Disconnected state it's the Features from the previous connected state.

There are also transitional states - when transitioning to a stable state:

  • Connecting (transitioning to Connected)
  • Disconnecting (transitioning to Disconnected)
  • Closing (transitioning to Closed)

We can't just discount these states and immediately "jump" to Connected or Closed. For example, when you call ConnectAsync, the behavior is and should be different when the state is Initial, Connected or Connecting (= you wait for the previous ConnectAsync to complete).

We need to provide methods for these state transitions (and we already do):

  • Abort: to transition to Closed (via Closing very briefly)
  • ShutdownAsync: to transition to Closed (via Closing)
  • ConnectAsync: to transition to Connected (via Connecting)
  • DisconnectAsync: to transition gracefully to Disconnected (via Disconnecting)

Some state transitions can occur without an explicit method call. For example, if the underlying connection drops, Connection transitions automatically to Disconnected or Closed.

@ReeceHumphreys ReeceHumphreys removed their assignment May 4, 2022
@bentoi
Copy link
Contributor Author

bentoi commented Jun 21, 2022

No longer relevant.

@bentoi bentoi closed this as completed Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Proposal for a new feature or significant update
Projects
None yet
Development

No branches or pull requests

3 participants