Repository navigation
Share a repeated evidence set at most once in each interval - #61
Draft
jwrosewell wants to merge 4 commits into
Draft
jwrosewell wants to merge 4 commits into
jwrosewell wants to merge 4 commits into
Conversation
The usage sharing tracker stores the time evidence was first shared and never moves it on. A repeat is held back for the interval after the first share, and after that it is shared on every request until the cache drops it. The specification describes the interval as a sliding window, which Python and Node follow by storing a new time whenever they share a repeat. The test shares evidence, waits past the interval, shares the repeat, and expects a repeat straight after that not to be shared. It fails until the tracker stores the time a repeat is shared.
The tracker now stores the time it shares a repeat in the cached Date, so the interval starts again from that share. A repeat is shared at most once in each interval, which is what the specification's sliding window describes and what Python and Node already do. The check and the update are made under the Date's lock, so two requests cannot both share the same repeat.
…reaches The tests shared a 50 millisecond interval, so on a slow runner two requests that should count as one repeat could be more than 50 milliseconds apart and both be tracked. ShareUsageTracker_Session_DoNotTrack failed that way on MacOS_X64_Java_11, with the test taking 2.4 seconds. Only the test that sleeps past the interval to test expiry keeps the short one. ShareUsageTracker_Session_Track no longer sleeps, so the session cookie alone keeps its two requests apart.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Java's repeat evidence interval works differently from the specification, Python and Node, in the same way as .NET's. This makes Java share a repeated evidence set at most once in each interval, as 51degrees-dotnet#87 does for .NET.
Problem
The session tracking section of the specification calls the repeat evidence interval a sliding window.
TrackerBase.track(TrackerBase.java lines 61 to 76) stores a time only for a new entry, andShareUsageTracker.match(ShareUsageTracker.java lines 72 to 76) never moves it on. A repeat is held back for 20 minutes after it was first shared, and after that it is shared on every request until the cache drops the entry. Python and Node store a new time when they share a repeat, so they share it at most once in each interval.Change
ShareUsageTracker.matchsets the cachedDateto the time it shares a repeat, so the next interval starts from that share. The cache holds theDateitself, so changing it in place is enough. The check and the change are made while holding theDate's lock, so two requests cannot both share the same repeat.Tests
ShareUsageTracker_RepeatEvidence_SharedOncePerIntervalin ShareUsageTrackerTests.java shares evidence, waits past a 500 millisecond interval, shares the repeat, and expects a repeat straight after that not to be shared.The full pull request build (run 42348034982) passed on every configuration, at head 5ee58c0.
Tracker test intervals
The full build of #63 (run 42423355619, first attempt) failed on MacOS_X64_Java_11 in
ShareUsageTrackerTests.ShareUsageTracker_Session_DoNotTrack, "expected: <1> but was: <2>" at line 184, with the test taking 2.405 seconds. Every test in the class used a 50 millisecond interval, and that test tracks two requests that differ only by the session cookie it leaves out, expecting one record. When the two requests are more than 50 milliseconds apart, which building their Mockito mocks on a busy runner can take, the second counts as a new share.ShareUsageTrackerTests.java now gives the tests that do not test expiry an interval of an hour (
NO_EXPIRY_INTERVAL_MILLIS). The short interval is kept only where a test sleeps past it, beingShareUsageTracker_RepeatEvidence_AfterSessionTimeoutat 50 milliseconds (EXPIRY_INTERVAL_MILLIS) andShareUsageTracker_RepeatEvidence_SharedOncePerIntervalat its own 500.ShareUsageTracker_Session_Trackno longer sleeps, so only the session cookie keeps its two requests apart, where before the expired interval could have done it.Each check below ran the class 50 times on macos-15-intel with JDK 11, each run in a new JVM.
rcat 2299507rcat 2299507ShareUsageTracker_Session_DoNotTrack"expected: <1> but was: <2>"Without load the failure did not appear, so the busy loops stand in for the full build's other work, which is when it was seen. Under load
ShareUsageTracker_Session_DoNotTracktook up to 4.17 seconds onrcand up to 2.485 seconds on this branch, andShareUsageTracker_RepeatEvidence_SharedOncePerInterval, which sleeps 750 milliseconds, took at most 0.911 seconds in all, so the calls after its sleep took at most 161 milliseconds, well inside its 500 millisecond interval.The branch has
rcmerged in at 2299507, which brings #52, #54 and #57 and touches none of these files.The full pull request build of the head, 4cf14b9, merged with
rc, passed all 15 build and test jobs on its first attempt (run 42437614160), on Ubuntu x64 with Java 8, 11, 17 and 21, Ubuntu arm64 with Java 17, Windows with Java 8, 11, 17 and 21, and macOS arm64 and x64 with Java 11, 17 and 21.Related