{"action":"created","issue":{"url":"https://api.github.com/repos/SeasideSt/Seaside/issues/848","labels_url":"https://api.github.com/repos/SeasideSt/Seaside/issues/848/labels{/name}","comments_url":"https://api.github.com/repos/SeasideSt/Seaside/issues/848/comments","events_url":"https://api.github.com/repos/SeasideSt/Seaside/issues/848/events","html_url":"https://github.com/SeasideSt/Seaside/issues/848","id":95528204,"number":848,"title":"errors during WABulkReapingCache>>reap can lead to \"corrupt\" cache in GemStone","user":{"login":"dalehenrich","id":623951,"avatar_url":"https://avatars.githubusercontent.com/u/623951?v=3","gravatar_id":"","url":"https://api.github.com/users/dalehenrich","html_url":"https://github.com/dalehenrich","followers_url":"https://api.github.com/users/dalehenrich/followers","following_url":"https://api.github.com/users/dalehenrich/following{/other_user}","gists_url":"https://api.github.com/users/dalehenrich/gists{/gist_id}","starred_url":"https://api.github.com/users/dalehenrich/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dalehenrich/subscriptions","organizations_url":"https://api.github.com/users/dalehenrich/orgs","repos_url":"https://api.github.com/users/dalehenrich/repos","events_url":"https://api.github.com/users/dalehenrich/events{/privacy}","received_events_url":"https://api.github.com/users/dalehenrich/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/SeasideSt/Seaside/milestones/1","html_url":"https://github.com/SeasideSt/Seaside/milestones/Seaside%203.2","labels_url":"https://api.github.com/repos/SeasideSt/Seaside/milestones/1/labels","id":1203765,"number":1,"title":"Seaside 3.2","description":"The Seaside 3.2 release focuses on improving session tracking performance and reducing the gap to the GemStone/S port, for more improvements check out the [changelog](https://github.com/SeasideSt/Seaside/wiki/Seaside320Changelog).","creator":{"login":"marschall","id":471021,"avatar_url":"https://avatars.githubusercontent.com/u/471021?v=3","gravatar_id":"","url":"https://api.github.com/users/marschall","html_url":"https://github.com/marschall","followers_url":"https://api.github.com/users/marschall/followers","following_url":"https://api.github.com/users/marschall/following{/other_user}","gists_url":"https://api.github.com/users/marschall/gists{/gist_id}","starred_url":"https://api.github.com/users/marschall/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/marschall/subscriptions","organizations_url":"https://api.github.com/users/marschall/orgs","repos_url":"https://api.github.com/users/marschall/repos","events_url":"https://api.github.com/users/marschall/events{/privacy}","received_events_url":"https://api.github.com/users/marschall/received_events","type":"User","site_admin":false},"open_issues":3,"closed_issues":30,"state":"open","created_at":"2015-07-12T08:11:55Z","updated_at":"2015-07-17T09:52:02Z","due_on":null,"closed_at":null},"comments":4,"created_at":"2015-07-16T21:01:06Z","updated_at":"2015-07-17T16:45:12Z","closed_at":null,"body":"As reported in [this GemStone issue](https://github.com/GsDevKit/Seaside31/issues/68) an error that occurs while removing the `expiredEntries` from the `dictionary` could cause *unreapable* entries to be stranded in the `dictionary`.  The entry is `unregistered` when it is added to the  `expiredEntries`, but if the  `expiredEntries` processing isn't completed due to an error, the entries will appear to be *expired* without having been removed form the `dictionary` which means that they could be *stuck forever* ...\r\n\r\nI haven't proposed a fix for https://github.com/GsDevKit/Seaside31/issues/68 but I think that the following would be a bit better:\r\n\r\n```Smalltalk\r\nreap\r\n\t\"Iterate through the cache and remove objects that have expired. Returns the number of expired objects.\"\r\n\t\r\n\t| now platform count expiredEntries |\r\n\tnow := Time totalSeconds.\r\n\texpiredEntries := OrderedCollection new.\r\n\t\r\n\tdictionary keysAndValuesDo: [ :key :value |\r\n\t\tvalue hasBeenAccessedSinceLastReap\r\n\t\t\tifTrue: [ value setAccessTime: now ].\r\n\t\t(self isExpired: value now: now)\r\n\t\t\tifTrue: [\r\n\"move to  \t\tself cacheEntryRemoved: value value.\"\r\n\t\t\t\texpiredEntries add: key ] \r\n\t\t].\r\n\t\r\n\t\"In GemStone, this method is performed by a separate maintenance VM, \r\n\t so we are already in transaction (assumed to be running in #autoBegin \r\n\t transactionMode) and do not have to worry about acquiring the TransactionMutex.\r\n\tSince we are using reducedConflict dictionaries in the first place, we will remove the keys\r\n\tand values from the existing dictionaries without using the mutex.\"\r\n\t\r\n\tcount := 0.\r\n\tplatform := GRPlatform current.\r\n\texpiredEntries do:[ :key |\r\n\t\tdictionary removeKey: key.\r\n\"here\"      self cacheEntryRemoved: value value\r\n\t\tcount := count + 1.\r\n\t\t(count \\\\ 100) isZero ifTrue: [\r\n\t\t\tplatform doCommitTransaction ] ].\r\n\tcount isZero ifTrue: [\r\n\t\tplatform doCommitTransaction ].\r\n\t^ count\r\n```\r\n\r\nThat will keep the *unregister* and dictionary in synch ... didn't look to see if this approach is practical ...\r\n\r\nAlso, it looks like there might be a missing commit if the `count` doesn't end on an an even 100 boundary ...\r\n\r\nBetter would be:\r\n\r\n```Smalltalk\r\n\t(count \\\\ 100) isZero ifFalse: [\r\n\t\tplatform doCommitTransaction ].\r\n```\r\n\r\nI think ..."},"comment":{"url":"https://api.github.com/repos/SeasideSt/Seaside/issues/comments/122336997","html_url":"https://github.com/SeasideSt/Seaside/issues/848#issuecomment-122336997","issue_url":"https://api.github.com/repos/SeasideSt/Seaside/issues/848","id":122336997,"user":{"login":"dalehenrich","id":623951,"avatar_url":"https://avatars.githubusercontent.com/u/623951?v=3","gravatar_id":"","url":"https://api.github.com/users/dalehenrich","html_url":"https://github.com/dalehenrich","followers_url":"https://api.github.com/users/dalehenrich/followers","following_url":"https://api.github.com/users/dalehenrich/following{/other_user}","gists_url":"https://api.github.com/users/dalehenrich/gists{/gist_id}","starred_url":"https://api.github.com/users/dalehenrich/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dalehenrich/subscriptions","organizations_url":"https://api.github.com/users/dalehenrich/orgs","repos_url":"https://api.github.com/users/dalehenrich/repos","events_url":"https://api.github.com/users/dalehenrich/events{/privacy}","received_events_url":"https://api.github.com/users/dalehenrich/received_events","type":"User","site_admin":false},"created_at":"2015-07-17T16:45:12Z","updated_at":"2015-07-17T16:45:12Z","body":"Thanks @marschall.\r\n\r\nI still think the final commit logic is wrong:\r\n\r\n```Smalltalk\r\n \t\tcount := count + 1.\r\n  \t\t(count \\\\ 100) isZero ifTrue: [\r\n  \t\t\tplatform doCommitTransaction ] ].\r\n \t(count \\\\ 100) isZero ifTrue: [\r\n  \t\tplatform doCommitTransaction ].\r\n  \t^ count\r\n```\r\n\r\nI think it needs to look like this:\r\n\r\n```diff\r\n \t\tcount := count + 1.\r\n  \t\t(count \\\\ 100) isZero ifTrue: [\r\n  \t\t\tplatform doCommitTransaction ] ].\r\n+ \t(count \\\\ 100) isZero ifFalse: [\r\n- \t(count \\\\ 100) isZero ifTrue: [\r\n  \t\tplatform doCommitTransaction ].\r\n  \t^ count\r\n```\r\nThis `(count \\\\ 100) isZero ifTrue:` is the same logic used in loop, so if the loop commits on the last count, the final commit will also commit .... if the loop does not commit on the last commit, the final commit will not commit as well. \r\n\r\nThe final commit is there to ensure that any *partial* work gets committed, so the lgic should invert the *in loop* logic..."}}