You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by svenmeier <gi...@git.apache.org> on 2017/09/10 08:38:14 UTC
[GitHub] wicket pull request #233: WICKET-6465 prevent unbound during storeTouchedPag...
GitHub user svenmeier opened a pull request:
https://github.com/apache/wicket/pull/233
WICKET-6465 prevent unbound during storeTouchedPages only
Why so complicated? We just want to prevent valueUnbound() from doing any harm while we're resetting the attribute during storeTouchedPages.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/svenmeier/wicket master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/wicket/pull/233.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #233
----
commit 8c7497144311923f7761e26eadedadfc9046f48e
Author: Sven Meier <sv...@apache.org>
Date: 2017-09-10T08:36:00Z
WICKET-6465 prevent unbound during storeTouchedPages only
----
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by bitstorm <gi...@git.apache.org>.
Github user bitstorm commented on the issue:
https://github.com/apache/wicket/pull/233
Don't forget to fix brackets indentation before commit
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by bitstorm <gi...@git.apache.org>.
Github user bitstorm commented on the issue:
https://github.com/apache/wicket/pull/233
Got it. That's way I hope for a future improvement of this boolean-based solution :-).
---
[GitHub] wicket pull request #233: WICKET-6465 prevent unbound during storeTouchedPag...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/wicket/pull/233
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on the issue:
https://github.com/apache/wicket/pull/233
Currently HttpSessionStore and PageStoreManager each use their own instance of HttpSessionBindingListener.
Yes, we might be able to unify these. But with a simple fix we don't introduce even more - possibly faulty - changes.
It was unfortunate that we didn't consider all possible callbacks from Servlet containers when this problem started with WICKET-6356.
IMHO we should go with the simplest fix: Prevent anything bad from happening, when re-setting the session attribute:
- clustering will work (because the attribute is re-set)
- it doesn't matter whether the container calls valueBound() or not
- if the container calls valueUnbound(), it does nothing bad during storeTouchedPages()
Andrea, what do you think?
BTW do we have to take care of concurrent access to the SessionEntry? I see that Martin used an AtomicBoolean, but how does that help if there are two request simultaneously storing touched pages?
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by papegaaij <gi...@git.apache.org>.
Github user papegaaij commented on the issue:
https://github.com/apache/wicket/pull/233
We don't care about concurrent calls to valueUnbound, that's fine. All code in that method is thread safe. What we don't want is multiple calls to storeTouchedPages and valueUnbound (session expiry) to interfere with each other. Synchronizing on the entry will not help as session expiry is probably done from a different thread.
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by papegaaij <gi...@git.apache.org>.
Github user papegaaij commented on the issue:
https://github.com/apache/wicket/pull/233
Yes, this is what I had in mind. I'm ok to merge this in 7.x and 8.
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by bitstorm <gi...@git.apache.org>.
Github user bitstorm commented on the issue:
https://github.com/apache/wicket/pull/233
> We don't care about concurrent calls to valueUnbound, that's fine. All code in that method is thread safe.
I agree
> What we don't want is multiple calls to storeTouchedPages and valueUnbound (session expiry) to interfere with each other. Synchronizing on the entry will not help as session expiry is probably done from a different thread.
Yes, synchronizing is meant to prevent problems for multiple invocations of storeTouchedPages for the same session, valueUnbound is unaffected. The boolean value should be enough to tell if valueUnbound is invoked as result of session expiration.
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by papegaaij <gi...@git.apache.org>.
Github user papegaaij commented on the issue:
https://github.com/apache/wicket/pull/233
Indeed, Sesison.destroy() is only called under certain conditions, and in those conditions, valueUnbound() will also be called.
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on the issue:
https://github.com/apache/wicket/pull/233
Agreed, a ThreadLocal makes sense.
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by bitstorm <gi...@git.apache.org>.
Github user bitstorm commented on the issue:
https://github.com/apache/wicket/pull/233
I agree about the use of a normal boolean, but I'm not sure I've understood your idea about ThreadLocal. What I would like to do is to prevent race conditions inside storeTouchedPages. I would do it using synchronized on entry object:
```
protected void storeTouchedPages(final List<IManageablePage> touchedPages)
{
if (!touchedPages.isEmpty())
{
SessionEntry entry = getSessionEntry(true);
synchronized (entry)
{
entry.setSessionCache(touchedPages);
for (IManageablePage page : touchedPages)
{
// WICKET-5103 use the same sessionId as used in SessionEntry#getPage()
pageStore.storePage(entry.sessionId, page);
}
entry.updating.set(true);
setSessionAttribute(getAttributeName(), entry);
}
}
}
```
In this way two possible concurrent requests for the same session would execute storeTouchedPages one at a time.
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by papegaaij <gi...@git.apache.org>.
Github user papegaaij commented on the issue:
https://github.com/apache/wicket/pull/233
No, a boolean won't work. Suppose the following happens: A thread calls storeTouchedPages, setting the boolean to true. At the same time, another thread unbinds the session, calling valueUnbound. This will leak the page store. We only want valueUnbound to be ignored when its a side effect of calling storeTouchedPages. This is local to the thread calling the method, hence ThreadLocal.
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by bitstorm <gi...@git.apache.org>.
Github user bitstorm commented on the issue:
https://github.com/apache/wicket/pull/233
I'm fine with this quick fix, but I warmly suggest to improve it after it has been applied, at least in master branch.
About concurrent access afaik we do have to take care of concurrency. At the moment we don't have any guarantee that setSessionAttribute saves the entry for the last request submitted (for the same session).
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by bitstorm <gi...@git.apache.org>.
Github user bitstorm commented on the issue:
https://github.com/apache/wicket/pull/233
We should consider method Session.destroy() to remove pages when session expires. See my comment at https://issues.apache.org/jira/browse/WICKET-6465
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by martin-g <gi...@git.apache.org>.
Github user martin-g commented on the issue:
https://github.com/apache/wicket/pull/233
When the session expires or is invalidated via API call then `#valueUnbound()` is called and this clears the storages.
This is how it worked last time I worked in this area of the code.
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by bitstorm <gi...@git.apache.org>.
Github user bitstorm commented on the issue:
https://github.com/apache/wicket/pull/233
I see. thank you for the clarification. Still, I'd like to evaluate more structural solutions, at least for Wicket 8. A good start point might be HttpSessionStore.SessionBindingListener#valueUnbound.
It's already used to call Session#onInvalidate and we can add Session#clear() after it.
---
[GitHub] wicket issue #233: WICKET-6465 prevent unbound during storeTouchedPages only
Posted by papegaaij <gi...@git.apache.org>.
Github user papegaaij commented on the issue:
https://github.com/apache/wicket/pull/233
I don't see why the current implementation uses `AtomicBoolean`. The current implementation could just as well use a normal boolean. If we need to take concurrent access into account (and I think we do), we should probably use a `ThreadLocal`. Of course this assumes that the servlet container will call (un)bound from the same thread, but the current implementation already has that assumption. If a container decides to make the calls async, there is no way of telling if it will happen between the setting and clearing of the boolean.
---