New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WICKET-6465 prevent unbound during storeTouchedPages only #233

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@svenmeier
Contributor

svenmeier commented Sep 10, 2017

Why so complicated? We just want to prevent valueUnbound() from doing any harm while we're resetting the attribute during storeTouchedPages.

@bitstorm

This comment has been minimized.

Show comment
Hide comment
@bitstorm

bitstorm Sep 11, 2017

Contributor

We should consider method Session.destroy() to remove pages when session expires. See my comment at https://issues.apache.org/jira/browse/WICKET-6465

Contributor

bitstorm commented Sep 11, 2017

We should consider method Session.destroy() to remove pages when session expires. See my comment at https://issues.apache.org/jira/browse/WICKET-6465

@martin-g

This comment has been minimized.

Show comment
Hide comment
@martin-g

martin-g Sep 11, 2017

Contributor

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.

Contributor

martin-g commented Sep 11, 2017

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.

@papegaaij

This comment has been minimized.

Show comment
Hide comment
@papegaaij

papegaaij Sep 11, 2017

Contributor

Indeed, Sesison.destroy() is only called under certain conditions, and in those conditions, valueUnbound() will also be called.

Contributor

papegaaij commented Sep 11, 2017

Indeed, Sesison.destroy() is only called under certain conditions, and in those conditions, valueUnbound() will also be called.

@bitstorm

This comment has been minimized.

Show comment
Hide comment
@bitstorm

bitstorm Sep 11, 2017

Contributor

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.

Contributor

bitstorm commented Sep 11, 2017

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.

@svenmeier

This comment has been minimized.

Show comment
Hide comment
@svenmeier

svenmeier Sep 11, 2017

Contributor

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?

Contributor

svenmeier commented Sep 11, 2017

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?

@bitstorm

This comment has been minimized.

Show comment
Hide comment
@bitstorm

bitstorm Sep 11, 2017

Contributor

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).

Contributor

bitstorm commented Sep 11, 2017

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).

@papegaaij

This comment has been minimized.

Show comment
Hide comment
@papegaaij

papegaaij Sep 12, 2017

Contributor

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.

Contributor

papegaaij commented Sep 12, 2017

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.

@bitstorm

This comment has been minimized.

Show comment
Hide comment
@bitstorm

bitstorm Sep 12, 2017

Contributor

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.

Contributor

bitstorm commented Sep 12, 2017

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.

@papegaaij

This comment has been minimized.

Show comment
Hide comment
@papegaaij

papegaaij Sep 12, 2017

Contributor

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.

Contributor

papegaaij commented Sep 12, 2017

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.

@bitstorm

This comment has been minimized.

Show comment
Hide comment
@bitstorm

bitstorm Sep 12, 2017

Contributor

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.

Contributor

bitstorm commented Sep 12, 2017

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.

@papegaaij

This comment has been minimized.

Show comment
Hide comment
@papegaaij

papegaaij Sep 12, 2017

Contributor

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.

Contributor

papegaaij commented Sep 12, 2017

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.

@bitstorm

This comment has been minimized.

Show comment
Hide comment
@bitstorm

bitstorm Sep 12, 2017

Contributor

Got it. That's way I hope for a future improvement of this boolean-based solution :-).

Contributor

bitstorm commented Sep 12, 2017

Got it. That's way I hope for a future improvement of this boolean-based solution :-).

@svenmeier

This comment has been minimized.

Show comment
Hide comment
@svenmeier

svenmeier Sep 12, 2017

Contributor

Agreed, a ThreadLocal makes sense.

Contributor

svenmeier commented Sep 12, 2017

Agreed, a ThreadLocal makes sense.

@papegaaij

This comment has been minimized.

Show comment
Hide comment
@papegaaij

papegaaij Sep 13, 2017

Contributor

Yes, this is what I had in mind. I'm ok to merge this in 7.x and 8.

Contributor

papegaaij commented Sep 13, 2017

Yes, this is what I had in mind. I'm ok to merge this in 7.x and 8.

@bitstorm

This comment has been minimized.

Show comment
Hide comment
@bitstorm

bitstorm Sep 13, 2017

Contributor

Don't forget to fix brackets indentation before commit

Contributor

bitstorm commented Sep 13, 2017

Don't forget to fix brackets indentation before commit

asfgit pushed a commit that referenced this pull request Sep 13, 2017

@asfgit asfgit closed this in 68b24aa Sep 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment