Mitigate random CookieStore.testConcurrentLoad test failures

[ato]

The arbitary value 25 was used but in practice it's quite possible
for more than 25 writing threads to have checked the cookie count
limit before adding their cookie. In practice we see Travis failing
on this test quite often, every few builds in fact.

I think using threads.length (i.e. 200) should cover the worst
case possibility where every thread reads a stale count and tries
to add their cookie. Edit: Updated to remove the test entirely.

Fixes #274

[anjackson]

The change looks fine, but I'm still a bit confused as to what this test is actually testing. If we make the upper-bound so large it can never possibly be exceeded, what's the point of the test?

ADDENDUM: Isn't this failure just showing that MAX_COOKIES_FOR_DOMAIN gets ignored? And that

heritrix3/modules/src/main/java/org/archive/modules/fetcher/AbstractCookieStore.java

Lines 288 to 300 in 0581170

	public void addCookie(Cookie cookie) {
	if (isCookieCountMaxedForDomain(cookie.getDomain())) {
	logger.log(
	Level.FINEST,
	"Maximum number of cookies reached for domain "
	+ cookie.getDomain() + ". Will not add new cookie "
	+ cookie.getName() + " with value "
	+ cookie.getValue());
	return;
	}
	addCookieImpl(cookie);
	}

should be synchronised?

[nlevitt]

The change looks fine, but I'm still a bit confused as to what this test is actually testing. If we make the upper-bound so large it can never possibly be exceeded, what's the point of the test?

I'm thinking you're right @anjackson. Maybe we should just drop the test. The assumption when we wrote the test was that a race condition would not be so frequent in practice. We've seen that under the contrived conditions created by the test case, it is frequent. But that's ok

[ato]

Andy writes:

Isn't this failure just showing that MAX_COOKIES_FOR_DOMAIN gets ignored?

My understanding is it doesn't get ignored, it just means under high thread contention it can be exceeded by up to the thread count, as each thread re-entering will recheck the count. I can't say I'm 100% confident of that understanding though.

Under normal configuration I don't think multiple threads will be writing to the same domain anyway, although I wouldn't rely on that were the side effects any worse.

Noah writes:

Maybe we should just drop the test.

I'm all for that. Updated to do so.

[nlevitt]

Thanks guys!