WICKET-6558 no lock after detach #380
Conversation
| pageManager = internalGetPageManager(); | ||
| } | ||
| pageManager.detach(); | ||
| internalGetPageManager().detach(); |
There was a problem hiding this comment.
Sufficient to detach the page manager only. No need to detach the session here, RequestCycle#onDetach() does it already.
|
|
||
| if (Application.exists()) |
There was a problem hiding this comment.
Left over when this code was copied over from a request listener.
| @@ -670,6 +676,9 @@ public void detach() | |||
| { | |||
| detachFeedback(); | |||
|
|
|||
| pageAccessSynchronizer.get().unlockAllPages(); | |||
| RequestCycle.get().setMetaData(PAGES_UNLOCKED, true); | |||
There was a problem hiding this comment.
MetaData is set on request cycle, so no need to clear the flag.
| @@ -915,6 +924,10 @@ public int nextPageId() | |||
| */ | |||
| public final IPageManager getPageManager() | |||
| { | |||
| if (Boolean.TRUE.equals(RequestCycle.get().getMetaData(PAGES_UNLOCKED))) { | |||
There was a problem hiding this comment.
When detached, Session no longer gives access to it's access-synchronizing manager.
| getLastRenderedPage().detach(); | ||
| getSession().detach(); | ||
| applicationSettings.setFeedbackMessageCleanupFilter(old); | ||
|
|
There was a problem hiding this comment.
We cannot detach the session, otherwise it will block access to its page manager afterwards. Actually detachment isn't needed anyways, just the message have to be cleared. For symmetry don't call detach on components either.
|
Looks good to me. We had some custom logging asking for the name of the page class after the session had already been detached. We only noticed it in production with locked page exceptions - this would have made debugging much easier. |
|
Hard to tell what might increase the likelihood of that happening. Perhaps you can pinpoint a specific version upgrade? |
| @@ -915,6 +924,10 @@ public int nextPageId() | |||
| */ | |||
| public final IPageManager getPageManager() | |||
| { | |||
| if (Boolean.TRUE.equals(RequestCycle.get().getMetaData(PAGES_UNLOCKED))) { | |||
| throw new WicketRuntimeException("pages have already been unlocked - synchronized access is no longer possible"); | |||
There was a problem hiding this comment.
I am not sure how understandable this message would be for the average developer. Maybe something like "The request [cycle] has been processed. Access to the pages is no more allowed" ?
A possible solution:
Session can prevent creating of locks after it has been detached for the end of the request.