Skip to content
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

Review ContextHandler locking #5088

Closed
sbordet opened this issue Jul 28, 2020 · 1 comment · Fixed by #5094
Closed

Review ContextHandler locking #5088

sbordet opened this issue Jul 28, 2020 · 1 comment · Fixed by #5094
Assignees

Comments

@sbordet
Copy link
Contributor

sbordet commented Jul 28, 2020

Spawned by #5083.

Jetty version
9.4.x

Description
ContextHandler uses a mix of synchronized modifiers and synchronized (this) inside methods that is difficult to reconcile.

Furthermore, it has a setClassLoader() method that allow to arbitrarily change the _classLoader field but the rest of the code does not protect with a lock this field, nor it reads the field only once.
For example, loadClass(), handle(), addListener(), etc. reference the _classLoader field multiple times and the field may change between the references.

Also, handle() could have an improved logic.
In handle(), in the finally block, rather than checking on oldClassLoader the check should be done symmetrically on _classLoader like the code before the try does and like doStart() does.

Also, setAvailable() is locked, but isAvailable() is not. In general, _availability is not correctly locked.

@joakime
Copy link
Contributor

joakime commented Jul 28, 2020

I vote that .setClassLoader(ClassLoader) should not be allowed after the ContextHandler is started/running.

Changing the classloader after some classes have been loaded cannot lead to good things.

@gregw gregw self-assigned this Jul 28, 2020
gregw added a commit that referenced this issue Jul 29, 2020
The locking was primarily as a memory guard for the availability status, which was already volatile.
Have instead using an AtomicReference with a simple state machine layered on top of start/stop lifecycle.
There was also protection for AttributesMap, which is no longer needed as AttributesMap is now concurrent.
gregw added a commit that referenced this issue Jul 29, 2020
updates from review
@gregw gregw linked a pull request Jul 29, 2020 that will close this issue
gregw added a commit that referenced this issue Jul 30, 2020
updates from review (better this time)
gregw added a commit that referenced this issue Jul 30, 2020
* Issue #5088 Review ContextHandler locking

The locking was primarily as a memory guard for the availability status, which was already volatile.
Have instead using an AtomicReference with a simple state machine layered on top of start/stop lifecycle.
There was also protection for AttributesMap, which is no longer needed as AttributesMap is now concurrent.

* Issue #5088

updates from review

* Issue #5088

updates from review (better this time)
@gregw gregw closed this as completed Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants