Fix timeout support #41
Assignees
Labels
bug
Something isn't working
Comments
crankydillo
pushed a commit
to crankydillo/mocca
that referenced
this issue
Sep 21, 2021
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
pushed a commit
to crankydillo/mocca
that referenced
this issue
Sep 21, 2021
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
pushed a commit
to crankydillo/mocca
that referenced
this issue
Sep 21, 2021
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
pushed a commit
to crankydillo/mocca
that referenced
this issue
Sep 21, 2021
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
pushed a commit
to crankydillo/mocca
that referenced
this issue
Sep 27, 2021
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
pushed a commit
to crankydillo/mocca
that referenced
this issue
Feb 5, 2022
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
pushed a commit
to crankydillo/mocca
that referenced
this issue
Feb 5, 2022
Includes generics fixes to the optional context for feign's async client. Honestly, we probably need to hide that in some way because I don't think we expose it.. I addressed one PMD issue that I understand, but I didn't really get the transient thing. Those aren't beans..
crankydillo
pushed a commit
to crankydillo/mocca
that referenced
this issue
Feb 7, 2022
crankydillo
added a commit
that referenced
this issue
Feb 17, 2022
* Fix timeout support #41 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. * The main part of this commit is covering tests for cases where... Mocca will govern timeout via its API. Also includes some review comments like access modifiers, javadoc, and gradle snafus. Still have async test cases to write. * Address timeout issue for async cases #41 Includes generics fixes to the optional context for feign's async client. Honestly, we probably need to hide that in some way because I don't think we expose it.. I addressed one PMD issue that I understand, but I didn't really get the transient thing. Those aren't beans.. * Test fixes I thought these got added with rebase:( * Remove unused async context type variable #41 Co-authored-by: Samuel Cox <sacox@paypal.com>
fabiocarvalho777
added a commit
to fabiocarvalho777/mocca
that referenced
this issue
Feb 17, 2022
fabiocarvalho777
added a commit
to fabiocarvalho777/mocca
that referenced
this issue
Feb 17, 2022
fabiocarvalho777
added a commit
that referenced
this issue
Feb 18, 2022
|
Code changes to fix this issue are in this branch: https://github.com/paypal/mocca/commits/fix_for_issue_41 These changes were originally already in develop branch. However, because they caused the error below, they were removed from develop branch and placed at branch fix_for_issue_41. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The fix for #7 is flawed. It implies (at least) 2 problematic things:
The first isn't necessarily true, and the second definitely isn't true with current feign impl. So basically, what this means is that for every
MoccaHttpClientother than the JAXRS2 one, where we ignore feign's attempt to set its default timeouts, the user is forced into feign's timeout defaults.The text was updated successfully, but these errors were encountered: