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

Bad assumption that the same classloader that loaded rocker loaded the templates. #4

Closed
ldaley opened this issue Dec 11, 2015 · 5 comments

Comments

@ldaley
Copy link
Contributor

ldaley commented Dec 11, 2015

The problem is here:

https://github.com/fizzed/rocker/blob/master/runtime/src/main/java/com/fizzed/rocker/runtime/DefaultRockerBootstrap.java#L52

This should be:

        return buildTemplate(modelType, model, modelType.getClassLoader());
@jjlauer
Copy link
Member

jjlauer commented Dec 11, 2015

@alkemist I suppose there may be an issue, but it's potentially not as easy of a solution as you proposed when you take the two flavors of hot reloading into account. Can you elaborate on why you dug into this in the first place and if so can you produce a unit test that fails? I'd be happy to help you get this addressed, but I'd need to understand the problem a bit more and definitely would like to prevent a regression down the road with a test case that can verify it.

@ldaley
Copy link
Contributor Author

ldaley commented Dec 11, 2015

I hit this when trying to use Gradle continuous build with Rocker and Ratpack. When running in that scenario, classes that may change (e.g. template class) are loaded in a child loader of a loader of the stable classes (e.g. Rocker impl).

This change should be safe as the loader in question is used to reflectively load an inner of the actual template. Therefore, using the loader that loaded the template makes sense.

ldaley added a commit to ldaley/rocker that referenced this issue Dec 11, 2015
@jjlauer
Copy link
Member

jjlauer commented Dec 15, 2015

I'll need to dig into this further to see what impact this has, but all your additional formatting changes makes it difficult to see exactly what you changed. If you could eliminate all your formatting changes (e.g. reversion to wildcard java imports, chopping newlines w/ spaces, etc.) and only submit PRs with actual lines of code that changed, I'd be happy to review this again.

@ldaley
Copy link
Contributor Author

ldaley commented Dec 16, 2015

Updated the PR with the minimal change.

@ldaley
Copy link
Contributor Author

ldaley commented Dec 16, 2015

I can't see how this would affect hot reloading as that uses a different impl of RockerBootstrap and supplies an entirely different loader to this method.

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

No branches or pull requests

2 participants