Specialize our safelyExecute calls to avoid logs

[niloc132]

Fixes #3277

[niloc132]

TODO added because this is the only remaining callsite for safelyExecute - like the python autocomplete code, I'm thinking we'll replace it with a try/catch. But do we want to log the error in this case, or are we only intending to propagate it to the client?

[nbauernfeind]

IMO, SessionState changes belong in a stream table to make client debugging a lot easier. I actually have a branch that does this. It has been helpful to debug api client issues for the c++ and go clients. I have not yet put it up for a PR as the web-ui is extremely annoying (re-creating exports of the lastBy version of the table makes the table unusable without also filtering out the web-ui's session). State change logging is extremely verbose and hard to follow from the log -- so the table is a real winner in that regard. If we only log errors here (and not requests and other state changes) then there is not enough context here to understand what failed or why. Most errors will have already logged if they were not due to API issues (i.e. uncaught exceptions will be logged, but illegal parameters and other StatusRuntimeException will not be logged).

I prefer safely ignoring any onError failure here.

[devinrsmith]

Looks great. Stronger interface allowing callers to not do their own lambdas is much cleaner.

[devinrsmith]

I don't think we've done this before... but do we have a responsibility to call observer.onError if observer.onNext throws?

[niloc132]

No, if it throws, the connection is gone, no need to tell it that the connection is gone.

[devinrsmith]

I wonder if it makes sense to add a comment to that effect.

At least the javadoc for StreamObserver#onNext has:

If an exception is thrown by an implementation the caller is expected to terminate the
stream by calling {@link #onError(Throwable)} with the caught exception prior to
propagating it.

[niloc132]

I believe @nbauernfeind and I went back and forth on this, but eventually dug into the implementation to confirm, since we found the docs to be vague. I double checked it now: in short:

• ServerCalls.ServerCallStreamObserverImpl.onNext checks if already canceled/aborted/completed and throws back to you if so. Clearly a caller of onNext need not call onError in this case.
• Next, ServerCallImpl.sendHeaders is called, which delegates to sendHeadersInternal to encode, and eventually delegates down to AbstractServerStream.Sink.writeHeaders. From experience, I can tell you that this method is not permitted to throw, bad things will happen and state will be broken.
• Last, ServerCallImpl.sendMessage delegates to sendMessageInternal catches its own exceptions from sending a payload out to transport, and calls close() on the stream.

Note that I'm only here concerned with calling onNext and onError on the instance of the StreamObserver provided by grpc to our service impl. If you are in a situation where you have both a responseStreamObserver and requestStreamObserver, and fail when calling one side's onNext, you are clearly obligated to call the other side's onError. We don't write a lot of code like that, but it is my guess that this case is what the javadoc is referring to.

[niloc132]

Pushed a commit that I think covers all the java changes I expect to make at this time - still going to add javadoc for the new calls.