stephenh / mirror

A tool for real-time, two-way sync for remote (e.g. desktop/laptop) development
Apache License 2.0
391 stars 35 forks source link

BlockingStreamObserver should honor cancelled state #1

Closed uweschaefer closed 7 years ago

uweschaefer commented 7 years ago

https://github.com/stephenh/mirror/blob/master/src/main/java/mirror/BlockingStreamObserver.java

I don't know this project and too little about the context in which the above class is used. Nevertheless, if used at the server, you might want to register the state-change hook not only as onReadyHandler, but also as onCancelHandler.

Furthermore, the while loop in onNext should probably be prepared for the connection being canceled as well. I use something like this:

       synchronized (lock) {
            while (!delegate.isReady()) {
                try {
                    while (!delegate.isCancelled() && !delegate.isReady()) {
                        log.debug("slow client, waiting");
                        lock.wait(30000);
                    }

                    if (delegate.isCancelled()) {
                        return;
                    }

                } catch (InterruptedException meh) {
                    // ignore
                }
            }
        }

Note that 'isCancelled' and 'setOnCancelHandler' are defined on ServerCallStreamObserver

i did not dare to fork/change your code, but feel free to incorporate the suggestions or just delete this issue.

uweschaefer commented 7 years ago

Oh, almost forgot - obviously your class served as an inspiration of how to tackle basic flow control with a grpc based solution. Thanks for that! :D

stephenh commented 7 years ago

Hi, thanks for the note!

I added some handling for on cancel handler; great catch, I didn't think of that.

I was just thinking of hacking a bit on this project, so this is a nice little clean up to sneak in.

Definitely let me know if you discover more gotchas/best-practices with grpc control flow, as it's fairly good-luck-roll-your-own AFAICT at the moment.