Github user mmakundi commented on the issue:
https://github.com/apache/wicket/pull/212
Thanks @manuelbarzi and Some findings:
1. I would remove the (non-Javadoc) and instead format those with
start-prefix /** like true javadoc and remove extra empty lines in the javadoc
2. All in all check for formatting and extra lines etc.
3. In case entries.offer fails, entryMap.remove is called before
pageStore.storePage. If we assume pageStore.storePage is slow, maybe it would
be good idea to postpone remove so that a new requiest during
pageStore.storePage can use the in-memory reference? Similar to what is
happening in PageSavingRunnable.run? Also this will make a great re-usable
method:
storePageAndRemoveFromMap(...) {
log.debug("Saving asynchronously: {}...", entry);
pageStore.storePage(sessionId, page);
entryMap.remove(getKey(entry));
}
4. Same as in (3.) to catch (InterruptedException e) {
log.error(e.getMessage(), e);; storePageAndRemoveFromMap(...); }
5. Please junit-test if any conflict occur if unbind(sessionId) is called
while there are entries in the queue for that session.
6. Please add tests also for borderline racing cases such as same instance
is returned for new request arriving before storing is complete.
7. I would rename AsyncPageStore -> AsynchronousPageStore for clarity and
symmetry (symmetric naming with AsynchronousDataStore),
8. Please propose a patch also into DefaultPageManagerProvider such that
new AsyncPageStore(super.newPageStore(dataStore), 100); would be the default
pagestore for wicket when dataStore.canBeAsynchronous()==true. Similar way that
AsynchronousDataStore is default.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
