Better Netty Logging and Error Reporting.

[scottweaver]

Added support to enable internal Netty log output.

Replaced contrived PrematureChannelException in NettyClientDriver with the actual underlying Netty exception.

[987Nabil]

@scottweaver The idea is good and needed. But I think the changes in the traits are not binary compatible.
Mima should give you some hints once it ran 🙂
Also I'd like @kyri-petrou to have a look.

[kyri-petrou]

Left some comments below, but my main concern is the failing MiMA checks. Since zio-http is in a stable version now, we need to ensure backwards binary compatibility at the very least for classes / methods that are publicly accessible and could be used by users.

[scottweaver]

Left some comments below, but my main concern is the failing MiMA checks. Since zio-http is in a stable version now, we need to ensure backwards binary compatibility at the very least for classes / methods that are publicly accessible and could be used by users.

The two things I see readily exposed to the end user, correct me id I missed something, are the additions of enableInternalLogging and onFailure to ClientDriver.requestOnChannel and enableInternalLogging added to ZClient#Config.

For the later, Config has a default value set for the new enableInternalLogging so that should limit its impact.

For the former, I would argue that requestOnChannel is used exclusively by zio-http internals and is ONLY public so that alternative implementations other than Netty could be created.

While it is not outside the realm of possibility someone, somewhere has actually done that, I would argue the vast majority of users have never even looked at the ClientDriver class much less even considered creating their own custom implementation.

[987Nabil]

Left some comments below, but my main concern is the failing MiMA checks. Since zio-http is in a stable version now, we need to ensure backwards binary compatibility at the very least for classes / methods that are publicly accessible and could be used by users.

The two things I see readily exposed to the end user, correct me id I missed something, are the additions of enableInternalLogging and onFailure to ClientDriver.requestOnChannel and enableInternalLogging added to ZClient#Config.

For the later, Config has a default value set for the new enableInternalLogging so that should limit its impact.

For the former, I would argue that requestOnChannel is used exclusively by zio-http internals and is ONLY public so that alternative implementations other than Netty could be created.

While it is not outside the realm of possibility someone, somewhere has actually done that, I would argue the vast majority of users have never even looked at the ClientDriver class much less even considered creating their own custom implementation.

While that might be true, I don't want to break compat if it is not absolutely necessary.
For requestOnChannel, please add default values, put the values at the end of the parameter list and use unroll. And for Config actually just the same 🙂

[scottweaver]

@987Nabil I went ahead and added Unroll and modified the methods.

The promise argument bit is mighty ugly, but it is what I came up for a first attempt. I welcome any input to clean that up.

[scottweaver]

I have removed the concerning await portion of the code, gonna put the commit comments here as a summary for the new implementation:

This set of changes improves on the initial pass in a couple of ways.

There is no longer a need to await on the 'onFailure' promise, reasons provided below.

All non-websocket channel handling operations now live inside 'ClientInboundHandler'. This includes the logic for 'exceptionCaught' which is responsible for properly completing 'onComplete', 'onResponse' and 'onFailure' when an exception occurs during Netty channel pipeline processing.

The 'ChannelInterface.interrupt' implementation now handles completion up of the 'onResponse' and 'onFailure' promises in the case where 'onComplete' is interrupted.

The close future listener has been completely removed as its responsibilities are covered by the 'ClientInboundHandler' and the 'ChannelInterface'. Interesting discovery for me was that the listener actually gets invoked BEFORE the 'exceptionCaught' method of 'ClientInboundHandler'. This is what lead be to the decision to completely remove the listener all together.

[scottweaver]

Well, I "fixed" some of the mima compat issues in my local env.

Unroll is working fine for requestOnChannel, however, it fails to rewrite ZClient.Config with a compiler crash.

I am wondering if Unroll doesn't like the nested case class?

Nvm, user error. I forgot to include the default value :(

[kyri-petrou]

How is this Promise used? I can't find any usage of await or poll on it, so AFAICT we're only completing it without really using it?

[kyri-petrou]

This existed in order to account for cases that the error is not propagated via the pipeline, e.g., if the server drops the connection unexpectedly or if we errored before reaching the ClientInboundHandler.

Have you tested what happens in those cases? Are we still fulfilling the promises or are we risking the application hanging?