search

paypal / mocca

Mocca is a GraphQL client for JVM languages with the goal of being easy to use, flexible and modular.
MIT License
15 stars 14 forks source link

Fix timeout support #41 #47

Closed crankydillo closed 2 years ago

crankydillo commented 2 years ago

See the issue for more info, but basically the code I'm replacing assumed that:

  1. Connect/read timeout was always done with client creation.
  2. Our impl (technically feign) couldn't successfully set those per request.

This work addresses the above and 3 issues:

  1. How a client can specify how the timeouts are specified.
  2. How the builder chain can honor the above in a nice way (i.e if you a client says it doesn't support request timeouts then those aren't exposed via the build chain.
  3. How to test without repetition.

So, it was just interesting to try and solve this. Now, I don't think this code is super-complicated, but I can understand distaste for this. I primarily did it just out of interest:) I think you could make a decent argument that we just bring back the timeout methods to all builder chains and in the jaxrs2+client (we could still add a builder option), we'd just log that these are ignored. That's unfortunate, but if this is the only case where we need to ignore request-level timeouts, then I can understand not wanting this code.

crankydillo commented 2 years ago

@fabiocarvalho777 I pushed some more javadoc. As we discussed, the main user impact here is to the builder. At a certain point, you supply a client (or use defaultClient()) where the client implementations leverage marker interfaces to determine whether or not the builder should expose timeouts or not. Most do expose timeouts. Currently, only JAX-RS does not.

This is not feature complete. I need to do something similar for async, finish up testing, and adjust user doc. However, the main idea is there, and I thought this might be controversial, so I didn't want to do any needless work. This was supposed to be opened as a draft, but I guess didn't do that the 2nd go around.

crankydillo commented 2 years ago

@fabiocarvalho777 This is my proposed fix. It does include a few other fixes, primarily related to the C in feign's AsyncClient<C>.

I did not update the end user doc. I am hoping you will be willing to do that after we decide if this is what we want to do.

I will fix any reasonable CI failures.