You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by agherardi <gi...@git.apache.org> on 2017/10/25 14:09:45 UTC
[GitHub] httpcomponents-client pull request #88: HTTPCLIENT-1855
GitHub user agherardi opened a pull request:
https://github.com/apache/httpcomponents-client/pull/88
HTTPCLIENT-1855
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/agherardi/httpcomponents-client HTTPCLIENT-1855
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/httpcomponents-client/pull/88.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #88
----
commit 8a107d6ff40edfe2f83c6655ae1d816584deb0c4
Author: alessandro.gherardi <al...@schneider-electric.com>
Date: 2017-10-23T00:02:11Z
HTTPCLIENT-1855
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #88: HTTPCLIENT-1855
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/88#discussion_r151871562
--- Diff: httpclient5/src/main/java/org/apache/hc/client5/http/auth/AuthCache.java ---
@@ -45,4 +45,8 @@
void clear();
+ boolean canCache(String name);
+
+ boolean needsUpdatingAfterReusing(String name);
--- End diff --
@agherardi So, you still insist on needing to update the cache on each and every message exchange?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #88: HTTPCLIENT-1855
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/88#discussion_r151940280
--- Diff: httpclient5/src/main/java/org/apache/hc/client5/http/auth/AuthCache.java ---
@@ -45,4 +45,8 @@
void clear();
+ boolean canCache(String name);
+
+ boolean needsUpdatingAfterReusing(String name);
--- End diff --
@agherardi So, we are happily back to the same point where we have started: a cache that requires an update on each and every operation is probably not a cache.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #88: HTTPCLIENT-1855
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/88#discussion_r152042431
--- Diff: httpclient5/src/main/java/org/apache/hc/client5/http/auth/AuthCache.java ---
@@ -45,4 +45,8 @@
void clear();
+ boolean canCache(String name);
+
+ boolean needsUpdatingAfterReusing(String name);
--- End diff --
@agherardi I do not have a problem with us not agreeing with one another but I do feel uncomfortable about adding two methods to a public interface that are specific to one particular auth scheme and completely unnecessary to start with.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #88: HTTPCLIENT-1855
Posted by agherardi <gi...@git.apache.org>.
Github user agherardi commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/88#discussion_r152082458
--- Diff: httpclient5/src/main/java/org/apache/hc/client5/http/auth/AuthCache.java ---
@@ -45,4 +45,8 @@
void clear();
+ boolean canCache(String name);
+
+ boolean needsUpdatingAfterReusing(String name);
--- End diff --
canCache() is not specific to a particular auth scheme. I think it's better than using an annotation because it allows one to control which schemes an auth cache implementation can store, without having to write a new scheme class and a new scheme factory. But since you're the man, if you would rather use the annotation that's fine by me.
needsUpdatingAfterReusing() is needed to have the system re-cache a digest scheme after reuse. As I explained in the scenario above, when a digest scheme is allocated to a request, it should not be reused until a successful response to that request is received.
The alternative is to change RequestAuthCache like in here https://github.com/apache/httpcomponents-client/pull/88/commits/8a107d6ff40edfe2f83c6655ae1d816584deb0c4#diff-9ba10ad1fe707824930ac6fb842c871b. That code I added calls targetAuthExchange.setState(AuthExchange.State.CHALLENGED). I can put that call inside an if (scheme is digest), so it's digest specific. The point is that either the cache or the DigestScheme need to be told when the scheme has been reused successfully and can be made available for new requests. Please let me know if you have a different suggestion.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #88: HTTPCLIENT-1855
Posted by agherardi <gi...@git.apache.org>.
Github user agherardi commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/88#discussion_r152026284
--- Diff: httpclient5/src/main/java/org/apache/hc/client5/http/auth/AuthCache.java ---
@@ -45,4 +45,8 @@
void clear();
+ boolean canCache(String name);
+
+ boolean needsUpdatingAfterReusing(String name);
--- End diff --
> a cache that requires an update on each and every operation is probably not a cache.
I disagree. Caching digest authentication information is useful to avoid network roundtrips, which are much mostly costly than updating a cache. Unfortunately, unlike "standard" caching usecases, reusing cached digest authentication information requires the cache to be updated.
I understand you disagree with me, and that's totally fine. All I'm asking for it the hooks into httpclient that allow me to add my custom auth cache.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #88: HTTPCLIENT-1855
Posted by agherardi <gi...@git.apache.org>.
Github user agherardi commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/88#discussion_r151874368
--- Diff: httpclient5/src/main/java/org/apache/hc/client5/http/auth/AuthCache.java ---
@@ -45,4 +45,8 @@
void clear();
+ boolean canCache(String name);
+
+ boolean needsUpdatingAfterReusing(String name);
--- End diff --
Yes. Consider the following scenario:
- The auth cache contains a DigestScheme for host H, with nonce=N and nonce count=1
- Thread A needs to send a request to host H. The thread retrieves the DigestScheme from the cache, increments nonce count to 2 and uses N to create an Authorization header for its HTTP request.
- Thread B also needs to send a request to host H. If the cache returns the same DigestScheme, thread B creates an Authorization header for its HTTP request with nonce=N and nonce count=3.
- If thread B sends its HTTP request before thread A sends its HTTP request, host H rejects thread B's request because the nonce count is 3 instead of 2.
IMO, a DigestScheme needs to be removed from the cache until a response is received from the server, so that no other thread can use the same nonce. If a successful response is received from the server, the DigestScheme can be re-cached with an updated nonce count.
I wrote a custom AuthCache that implements the behavior above. The cache stores AuthSchemes unserialized. The cost of un-caching and re-caching DigestSchemes for every message exchange is minimal, especially when compared to the cost of a network roundtrip if the request needs to be resent due to the nonce count being out-of-sequence.
The needsUpdatingAfterReusing method allowed me to implement the custom AuthCache, which is not part of this merge request. BasicAuthCache's implementation of needsUpdatingAfterReusing returns FALSE, so BasicAuthCache is not updated on very message exchange - which is what you want.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org