vertx: end responses safely

[michael72]

this is regarding my issue #3157

The consequence of these changes is that (at least for VertxFutureServerInterpreter) the exception java.lang.IllegalStateException: Response has already been written does not occur any more.

The question is if the other changes are - maybe a bit too much?
One thing about the logging: I added slf4j logging because vertx logger should not be used directly.
See: eclipse-vertx/vert.x#2774

[kciesielski]

@michael72 thanks a lot for this fix! I am not sure about hardwiring to slf4j. What if we leave the logging entirely? There are 2 usages:

1. Error logging on catching Throwable in safeEndWait - is it ok to swallow the exception here? What if it just falls through? Or was the original problem in the first place?
2. In handleError, as an additional operation - wouldn't rc.fail(ex) cause some kind of logged error on another level?

Another minor thing is the hardcoded duration for safeEndWait. It is a default value which cannot be overridden be the user anyway. Maybe it should be read from VertxServerOptions (and set to a default value there)?

[michael72]

@kciesielski

Thanks for the review!

• safeEndWait -> just letting the exception through is probably the better option. It wasn't the problem in the first place, but simply ignoring the future looked a bit dubious here and could have lead to some problems
• rc.fail seems like does not invoke an extra logging, but it will trigger failure or error handlers - and they may or may not do logging. The handling here was also partly doing logging and partly not. Seems like there wasn't a real convention here.

btw it looks like debugLog and infoLog are not used anywhere in the code and they both use the deprecated api - hm.

[adamw]

@michael72 The Await was troubling me a bit, so I took a shot at actually changing the implementation so that it returns a vertx Future, and then converting it to the effect type. That way ZIO/cats should wait with the effect completion until the response is written. Also in case when the response is already ended, I'm returning an already completed future so that things don' tblock. Take a look and let me know if this makes sense :)

[michael72]

@adamw I think this looks good! I cannot say much about the zio/cats stuff though.
Cheers!

[adamw]

Good, let's try then ;)