Fix Client overwriting its connection cache when using a custom Net.ctx

[MisterDA]

When given a custom context, the client would always overwrite its connection cache. This prevents using a custom connection cache and a custom context. Access the connection cache directly.

[MisterDA]

I'm not convinced that this patch is complete, if the ctx changes, should the cache be recreated? It's probably not expected though, and it's tied to the problem of hiding a global ref behind the scenes…

[rgrinberg]

Can't be of much help here as I have no idea how any of this is supposed to work. A test would be useful to understand what this is changing though

[mseri]

Ping @madroach, can you help us reviewing this?

[madroach]

Hi @MisterDA,
I believe I understand your problem. You set a connection cache using set_cache, which won't be used if you use a custem Net.ctx. This behaviour is necessary, although maybe unintuitive, because a Net.ctx is hiding in the connection cache, which would be used instead of the provided custom Net.ctx.

If you want to use a custom Net.ctx with a connection cache, the way to achieve this is to create a connection cache providing your custom Net.ctx and then use this connection cache; possibly setting it as new default via set_cache. And do not again pass the Net.ctx to the call function.

This could maybe be improved upon by removing the ctx optional argument from the cache and various call functions altogether and thereby forcing users to go through a custom connection cache. This would make the semantics clearer, although remove some convenience. Another idea would be to simply put the connection cache into the Net.ctx ?

[MisterDA]

Thanks for your clear explanation of my problem and the possible solutions!

And do not again pass the Net.ctx to the call function.

Yes, I fell right into that. Should it be documented somehow? I find this behavior a bit surprising.

Another idea would be to simply put the connection cache into the Net.ctx?

This would make sense, I think. I find it quite confusing to have a global cache hidden when calling the Client.xxx functions. But then, if both the connection cache and the Net.ctx have references to each other, and it would sound a bit like a design flaw…

I'll close this PR as this patch isn't correct, but we can keep on discussing.

[mseri]

Yes, I fell right into that. Should it be documented somehow? I find this behavior a bit surprising.

Sounds like a very good idea. Could you suggest an edit for the docstrings?

[mseri]

This would make sense, I think. I find it quite confusing to have a global cache hidden when calling the Client.xxx functions. But then, if both the connection cache and the Net.ctx have references to each other, and it would sound a bit like a design flaw…

I agree, sounds like a good idea to just have it in the Net.ctx. Would that impose a breaking change only for the lwt backend or for all the backends?

[madroach]

I did something similar. Since ctx is abstracted in Cohttp.Generic.Client, it can easily be changed just in Cohttp_lwt.Client.
This will obviously break any code using ctx in Cohttp_lwt.Client. I did this change in #1075 as a proof of concept.

[madroach]

Yes, I fell right into that. Should it be documented somehow? I find this behavior a bit surprising.

It is documented. See the Cohttp_lwt.S.Client.ctx.

[madroach]

A downside of the approach to change ctx in Cohttp_lwt.Client is that it is confusing that ctx has a different type just in Cohttp_lwt.Client.