Implemented thread-safe CookieStore #1494

unoexperto · 2018-01-11T14:28:00Z

@slandelle I copied your unitests from gattling too. I kept extra methods in CookieStore besides add and get you and I discussed. I actively use remove, getAll and clear because I heavily use AHC for web scraping and serialize and manipulate cookies between sessions while maintaining connection to HTTP server.

If everything looks good to you could you please push new build to Maven ? I need it in production :)

Thanks!

slandelle · 2018-01-11T15:12:34Z

What have you done with the poms??? Please revert.

This reverts commit 094faaa

unoexperto · 2018-01-11T15:43:07Z

@slandelle Done :)

slandelle · 2018-01-11T15:14:18Z

client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java

+import java.util.concurrent.locks.ReentrantLock;
+import java.util.stream.Collectors;
+
+public class ThreadSafeCookieStore implements CookieStore {


please make final

slandelle · 2018-01-11T15:46:33Z

client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java

+    public boolean remove(Cookie cookie) {
+        try {
+            lock.lock();
+            return cookieJar.entrySet().removeIf(v -> v.getValue().cookie == cookie);


You mean equals, not ==.
Then, I suspect the predicate is wrong and one want to remove by (domain, path, name, value) and don't take other fields into account. Letting user pass a predicate instead would probably be better.

No, I intentionally wanted to remove by reference because intention is to modify cookie jar using Cookie objects returned by .getAll() or get() methods. This is similar to what java.net.InMemoryCookieStore.remove() does.

I think this is specific to your use case, Other people might want a different strategy, eg drop all cookies for a given domain. So it might be best to change the parameter to a Predicate<Cookie>.

slandelle · 2018-01-11T15:50:21Z

client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java

+        if (!requestUri.getPath().isEmpty())
+            return requestUri.getPath();
+        else
+            return "/";


Please use ternary operator ?.

Done. Forgot it exists in Java after years in Scala :)

slandelle · 2018-01-11T15:54:04Z

client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java

+            final boolean[] removeExpired = {false};
+            List<Cookie> result = cookieJar
+                    .entrySet()
+                    .parallelStream()


Why do you need a parallelStream here? Looks overkill, all the more as most users would only have a few cookies at the same time.

Not really. Let me downgrade then.

slandelle · 2018-01-11T15:55:07Z

client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java

+    public List<Cookie> getAll() {
+        try {
+            lock.lock();
+            final boolean[] removeExpired = {false};


Why do you need an array instead of a mutable boolean?

Variables used in lambda have to be final or effectively final so you can't modify them. Sad limitation of Java8.

slandelle · 2018-01-11T17:10:44Z

client/src/main/java/org/asynchttpclient/filter/FilterContext.java

+     * @param requestFilters {@link List<RequestFilter>}
+     * @return {@link FilterContext}
+     */
+    public FilterContext<T> preProcessRequest(List<RequestFilter> requestFilters) throws FilterException {


Unused, please revert

slandelle · 2018-01-11T17:11:34Z

...nt/src/main/java/org/asynchttpclient/netty/handler/intercept/ResponseFiltersInterceptor.java

 import org.asynchttpclient.AsyncHandler;
 import org.asynchttpclient.AsyncHttpClientConfig;
 import org.asynchttpclient.HttpResponseStatus;
+import org.asynchttpclient.cookie.CookieStore;


Please revert changes here

slandelle · 2018-01-11T17:12:27Z

client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java

+
+        final boolean[] removeExpired = {false};
+
+        List<Cookie> result = cookieJar.entrySet().parallelStream().filter(pair -> {


why parallelStream?

slandelle · 2018-01-11T17:12:33Z

client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java

+
+    private List<Cookie> get(String domain, String path, boolean secure) {
+
+        final boolean[] removeExpired = {false};


why an array?

Limitation of Java8 lambdas.

Damn. OK. I guess a array of booleans of size 1 is smaller than an AtomicBoolean.

slandelle · 2018-01-11T17:13:25Z

client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java

+public class ThreadSafeCookieStore implements CookieStore {
+
+    private Map<CookieKey, StoredCookie> cookieJar = new HashMap<>();
+    private ReentrantLock lock = new ReentrantLock();


What are the pros of this HashMap + ReentrantLock? Why not a ConcurrentHashMap?

I actually used ReadWriteLock first but then thought it's overkill so I replaced it with simpler class. I can use ConcurrentHashMap if you prefer that.

unoexperto · 2018-01-11T18:12:34Z

Made minor modifications that you've requested.

slandelle · 2018-01-11T18:17:56Z

client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java

+public final class ThreadSafeCookieStore implements CookieStore {
+
+    private Map<CookieKey, StoredCookie> cookieJar = new HashMap<>();
+    private ReentrantLock lock = new ReentrantLock();


Comments were lost when you pushed. Could you please change to a CHM?

slandelle · 2018-01-11T18:20:18Z

client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java

+        } else {
+            // rfc6265#section-5.1.4
+            if (!requestPath.isEmpty() && requestPath.charAt(0) == '/' && requestPath.lastIndexOf('/') > 0)
+                return requestPath.substring(0, requestPath.lastIndexOf('/'));


Not exactly my commit: you're computing lastIndexOf('/') twice :)

Crap, my bad. I pushed the fix.

unoexperto · 2018-01-11T18:21:24Z

Updated definition of remove()

slandelle · 2018-01-11T18:57:30Z

client/src/main/java/org/asynchttpclient/filter/FilterContext.java

@@ -151,4 +151,4 @@ public Request getRequest() {
        }
    }

-}
+}


Please restore last empty line

slandelle · 2018-01-11T18:57:46Z

Almost there :)

slandelle · 2018-01-11T19:35:08Z

@unoexperto Great! One last thing: could you please squash your commit into a single one and add a commit message such as described here: https://netty.io/wiki/writing-a-commit-message.html?

unoexperto · 2018-01-11T21:28:14Z

Took me an hour to create and apply single patch :( Not sure how to amend this PR so I create new clean one #1495

slandelle · 2018-01-11T21:45:44Z

@unoexperto

git rebase -i HEAD~2
r for renaming commit
f for fixing it up
then git push -f

unoexperto added 2 commits January 11, 2018 17:23

Implemented ThreadSafeCookieStore

6587c46

Upgraded version

094faaa

Revert "Upgraded version"

012579a

This reverts commit 094faaa

slandelle requested changes Jan 11, 2018

View reviewed changes

unoexperto added 2 commits January 11, 2018 21:08

Reverted unnecessary changes

9edb7ca

Minor optimizations

ab63ed9

Modified remove() to accept predicate instead of cookie object.

1b89a9d

slandelle requested changes Jan 11, 2018

View reviewed changes

unoexperto added 2 commits January 11, 2018 21:23

Compute lastIndex once instead of twice

889f792

Replaced external lock with change to ConcurrentHashMap

8771555

slandelle requested changes Jan 11, 2018

View reviewed changes

Revert of cosmetic change

a3b9c87

unoexperto closed this Jan 11, 2018

unoexperto deleted the cookie_store2 branch January 11, 2018 21:28


		final boolean[] removeExpired = {false};

		List<Cookie> result = cookieJar.entrySet().parallelStream().filter(pair -> {


		private List<Cookie> get(String domain, String path, boolean secure) {

		final boolean[] removeExpired = {false};

@@ @@ -151,4 +151,4 @@ public Request getRequest() { @@
                       }
                   }
-              }
+              }

Implemented thread-safe CookieStore #1494

Implemented thread-safe CookieStore #1494

Uh oh!

Conversation

unoexperto commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slandelle commented Jan 11, 2018

Uh oh!

unoexperto commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unoexperto commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unoexperto commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slandelle commented Jan 11, 2018

Uh oh!

slandelle commented Jan 11, 2018

Uh oh!

unoexperto commented Jan 11, 2018

Uh oh!

slandelle commented Jan 11, 2018

Uh oh!

Uh oh!

unoexperto commented Jan 11, 2018 •

edited

Loading