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

Consider handling specific exception in the Client.connect #20

Open
e5l opened this issue Dec 17, 2024 · 2 comments
Open

Consider handling specific exception in the Client.connect #20

e5l opened this issue Dec 17, 2024 · 2 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@e5l
Copy link
Collaborator

e5l commented Dec 17, 2024

Consider handling specific exception in the Client.connect:

    override suspend fun connect(transport: Transport) {
        // ...
        } catch (error: Throwable) {
            close()
            throw error
        }
    }
@e5l e5l added the enhancement New feature or request label Dec 17, 2024
@e5l e5l added the good first issue Good for newcomers label Jan 7, 2025
@Skrilltrax
Copy link

Hi @e5l, I was taking a look at the issue and figured out that there are a lot of exceptions that could get thrown here (especially because the request() method in the try block does json parsing and more). I think one better approach could be that we catch a generic Throwable that we are doing now but instead of throwing an Error, we can instead throw an IllegalStateException with the a message like Error connecting to transport and pass the original error throwable along with it. What do you think about this approach?

    override suspend fun connect(transport: Transport) {
        super.connect(transport)

        try {
            /* ... */
        } catch (error: Throwable) {
            close()
            if (error !is CancellationException) {
                throw IllegalStateException("Error connecting to transport", error)
            } else {
                throw error
            }
        }
    }

@e5l
Copy link
Collaborator Author

e5l commented Mar 5, 2025

Hey, @Skrilltrax, thanks for the investigation. I would also include the transport information in the message (and some unit tests as well). In general, it should be better than what we have right now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants