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

Investigate documented ConnectionException thrown by Publisher.Builder.start and how users are meant to handle it #876

Closed
lawrence-forooghian opened this issue Jan 4, 2023 · 5 comments · Fixed by #893
Assignees

Comments

@lawrence-forooghian
Copy link
Collaborator

lawrence-forooghian commented Jan 4, 2023

The documentation for Publisher.Builder.start states that it can throw a ConnectionException "if something goes wrong during connection initialization":

* @throws ConnectionException If something goes wrong during connection initialization

It seems like this is because DefaultAbly can throw a ConnectionException "if something goes wrong during Ably SDK initialization".

* @throws ConnectionException if something goes wrong during Ably SDK initialization.

We should understand what are the circumstances under which this might happen and explain to users how they are meant to recover from this situation. Can we handle it in a way that’s consistent with #871?

@sync-by-unito
Copy link

sync-by-unito bot commented Jan 4, 2023

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3213

@lawrence-forooghian lawrence-forooghian changed the title Investigate documented ConnectionException thrown by Publisher.Builder and how users are meant to handle it Investigate documented ConnectionException thrown by Publisher.Builder.start and how users are meant to handle it Jan 4, 2023
@paddybyers
Copy link
Member

We should understand what are the circumstances under which this might happen and explain to users how they are meant to recover from this situation. Can we handle it in a way that’s consistent with #871?

I don't think this should throw ConnectionException if the underlying connection is in any of the following states:
connecting
connected
disconnected
suspended.
In these cases, the underlying library will continuously retry the connection, and it should be possible via the Publisher to observe this, and any eventual successful connection.

There's an argument for throwing an exception if the underlying connection is in the closed or failed state.
closed will only ever be reached if there is an explicit call to close() on the underlying connection, which shouldn't be possible.

failed will happen if there is a non-retriable error in the underlying connection (eg the credentials are not valid). The underlying connection will never autonomously recover from this state and it's probably reasonable for the AAT Publisher to throw to indicate that it will never connect. (There's also an argument, in this case, for attempting to determine why the connection is failed, and have exception types that reflect those underlying causes, eg AuthorizationException or something - ConnectException isn't that informative, because it implies that there was a problem with this connection attempt, rather than a more concrete problem.)

@davyskiba davyskiba self-assigned this Jan 10, 2023
@davyskiba
Copy link
Contributor

I think this exception might be only thrown here which repackages AblyExceptions thrown by
Hosts constructor.

If I'm reading this correctly, it checks if the connection configuration is valid. Connection configuration is created here and is using default values for all fields reviewed by Hosts constructor. There is no way for the SDK user to change those, so I think these exceptions cannot be thrown when using AAT, and this try..catch along with exception annotation can safely be removed.

@KacperKluka can you confirm if that is correct?

@KacperKluka
Copy link
Contributor

The Publisher.Builder.start throws a ConnectionException because it creates an Ably wrapper instance which on the other hand creates an AblyRealtime instance. The AblyRealtime constructor declares to throw AblyException and it creates a bunch of objects from which some of them declare to throw the exception as well (e.g. Hosts, Auth, HttpCore).

Initially, the try/catch was added when we were looking for AblyRealtime methods that can throw and we wanted to make sure we catch all AblyExceptions.

@davyskiba
Copy link
Contributor

Thanks for pointing out more possible causes.

After looking at those classes, those exceptions can occur when using AAT, but they all seem related to the connection configuration validation. Could we change the exception comment to indicate that the cause is configuration related? @lawrence-forooghian @paddybyers @QuintinWillison wdyt?

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