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

Thoughts on adding support for connectivity state #186

Closed
SebastianThiebaud opened this issue Mar 14, 2018 · 12 comments
Closed

Thoughts on adding support for connectivity state #186

SebastianThiebaud opened this issue Mar 14, 2018 · 12 comments

Comments

@SebastianThiebaud
Copy link
Contributor

This Swift library doesn't expose connectivity states and doesn't include an API to watch state changes: https://github.com/grpc/grpc/blob/master/doc/connectivity-semantics-and-api.md

@dcow
Copy link

dcow commented Mar 14, 2018

+1. We would like to inform a user/client about the state of a channel at any given time (basically build a channel state UI, nothing more).

@MrMage
Copy link
Collaborator

MrMage commented Mar 14, 2018

Agreed. I am also working on improving the library's error reporting. For example, if you try to connect to an endpoint that simply does not exist, the library will let you send data just fine in streaming requests and simply throw an .endOfStream error when trying to read responses. It should instead inform the user earlier that the connection failed.

MrMage added a commit to Timing-GmbH/grpc-swift that referenced this issue Mar 19, 2018
@MrMage
Copy link
Collaborator

MrMage commented Mar 19, 2018

I'm adding a simple API for this in #188 (e68cec0).

@SebastianThiebaud
Copy link
Contributor Author

SebastianThiebaud commented Mar 20, 2018

I've been looking at your PR @MrMage and thanks for being on top of this. I've been working on a different implementation in my fork matching a little bit more what gRPC Python is offering with observers. What are your thoughts on updating the API to use observers?
https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/_channel.py#L808

@MrMage
Copy link
Collaborator

MrMage commented Mar 22, 2018

My concern with adding an observer is that it adds another polling thread. So running that extra thread should at least be optional I think.

@dcow
Copy link

dcow commented Mar 22, 2018

FWIW grpc-java punts on an observation mechanism too for exactly the same rationale as @MrMage. You'd need to "lock the world" to deliver connectivity state information in a way that's guaranteed to be accurate. From what I've gathered the compromise was to have a way to get a callback when a provided state is no longer the current state. You "watch" the provided state and get a callback when it changes. If the channel state has already changed from the provided state, the callback fires immediately, see: grpc/grpc-java#3763

@MrMage
Copy link
Collaborator

MrMage commented Mar 22, 2018

@dcow sounds interesting. When do these state-change events occur get noticed (and the corresponding callbacks called) in the Java API?

@dcow
Copy link

dcow commented Mar 22, 2018

I don't know the details. Basically you ask a channel to tell you when its state changes from the state you pass in. So I'd say, "channel tell me when you're no longer in the CONNECTED state by executing this here closure". So I'm guessing that state+action tuple ends up in a list somewhere and when the channel notices it has changed states it synchronously grabs/copies (by ref is okay) the list of observers, "clears" the old list, and async enqueues something iterates over the copied list and calls the closures?

@SebastianThiebaud
Copy link
Contributor Author

SebastianThiebaud commented Mar 22, 2018

Yes, it's exactly what I was suggesting with observers (what the C lib is calling watcher). Here is a sample code of my implementation and how I use it

        service.channel.subscribe(sourceState: .idle, tryToConnect: false) { state in
            print("from idle to \(state)")
        }

        service.channel.subscribe(sourceState: .ready, tryToConnect: false) { state in
            print("from ready to \(state)")
        }

        service.channel.subscribe(sourceState: .connecting, tryToConnect: false) { state in
            print("from connecting to \(state)")
        }

        service.channel.subscribe(sourceState: .initialized, tryToConnect: false) { state in
            print("from initialized to \(state)")
        }

        service.channel.subscribe(sourceState: .transientFailure, tryToConnect: false) { state in
            print("from transientFailure to \(state)")
        }

        service.channel.subscribe(sourceState: .shutdown, tryToConnect: false) { state in
            print("from shutdown to \(state)")
        }

@MrMage
Copy link
Collaborator

MrMage commented Mar 23, 2018

Can you share that code so we can look at it? :-)

@timburks
Copy link
Member

timburks commented May 26, 2018

What would be a good way to verify this (in tests)?

When this was verified during development, how many states were observable?

@SebastianThiebaud
Copy link
Contributor Author

Closing this issue since #196 and Timing-GmbH@e68cec0 are adding support for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants