Cookiejar optimization

[sivaalli]

Closes: #1580

Changes:

1. Segmented map based on domain names. So instead of traversing all the domains it traverses the domains that are of interest.
2. Use NettyTimer to clean up the expired cookies asynchronously. The timer task that provides this functionality is CookieEvictionTask.

[sivaalli]

@slandelle Please let me know if this looks good. Thanks

[slandelle]

Nice work.

However, I have some concerns regarding CookieStore handling its own scheduling.

For example, someone spawning multiple AHC instances with different configs would spawning multiple threads for nothing.
Or someone could want to share a single CookieStore amongst multiple instances with different configurations and would spawn multiple ThreadSafeCookieStores for nothing in DefaultAsyncHttpClientConfig#Builder (prior to overriding).

I think it would be better to use AHC's HashedWheelTimer instance for scheduling.

[sivaalli]

Thanks for the feedback. Let me change code to use HashedWheelTimer.

[slandelle]

Sorry, I'm not happy with the current design. I'm still convinced Timer and CookieStore should be isolated.

[sivaalli]

@slandelle Please let me know what you think. Thanks.

[slandelle]

Looking good. Now, I think the last missing piece is to handle CookieStore sharing from multiple AHC instances. Currently, if we share a CookieStore, we'll end up scheduling cleaning multiple times, and never stop scheduling.

I suggest we use an AtomicInteger to keep track of AHC instance a given CookieStore is configured on, only start scheduling when starting first client and only stop scheduling when closing last one.

WDYT?

[sivaalli]

Sounds good. Will start working on that.

[slandelle]

Thanks a lot for your great work!

[sivaalli]

@slandelle I've a few questions that I would like to get your opinion (Both cases assume sharing cookie store).

1. In case of shared Timer supplied by the user. We just maintain a counter on cookiestore and start scheduling if it is the first client and user takes care of stopping netty timer (hopefully after stopping all AHC instances). No issue with this situation.
2. If user did not supply Timer then we create new timer for each AHC instance. We can start scheduling only if it is first client on the first netty timer. But as soon as the user closes the first netty timer we would close the timer. But if user decides to use other (second and so on) AHC instances then we could never evict expired cookies.

I'm assuming that you made the comment above assuming that user shares a timer? Please correct me if my assumption is wrong.

If user does not share timer, then I guess we have to schedule eviction task on each timer.

Also a follow up question regarding creating new Timer for each AHC instance. I feel like we could share the same Timer for multiple AHC's, but I think there might be a strong reason on why not too. But just curios to know. Timer starts to emit error logs when the count is greater that 64.

[sivaalli]

I've made some changes assuming you are talking about shared netty timer. Please let me know how this looks. Happy to implement any of your suggestions.

I do not know if sharing netty timer across all AHC instances is useful or harms the overall performance (because all tasks share same timer). But if you'd like to see sharing a single netty timer (instead of creating a new one for each instance) in-case a user does not provide one, I can make that change as well.

[slandelle]

I'm assuming that you made the comment above assuming that user shares a timer? Please correct me if my assumption is wrong.

Yes

If user does not share timer, then I guess we have to schedule eviction task on each timer.

Let's do that

Also a follow up question regarding creating new Timer for each AHC instance. I feel like we could share the same Timer for multiple AHC's, but I think there might be a strong reason on why not too. But just curios to know. Timer starts to emit error logs when the count is greater that 64.

Could you please share the error logs?

[sivaalli]

I've personally have not created too many instances to see this message. But I was checking through source code of HasedWheelTimer and found that it spits out an error log when number of instances are greater than 64. Logs like this:

You are creating too many HashedWheelTimer instances. HashedWheelTimer is a shared resource that must be reused across the JVM, so that only a few instances are created.

The changes are made according to your expectation. Please let me know otherwise.

I can make the change to share netty timer if you are interested.

[slandelle]

@sivaalli Thanks!