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

Global context #447

Merged
merged 7 commits into from Jun 8, 2020
Merged

Global context #447

merged 7 commits into from Jun 8, 2020

Conversation

hs-lsong
Copy link
Collaborator

JinJava can be a singleton and shared by multiple threads. The JinJava rendering context is scoped, but the root of the context -- the global context -- is shared. However, the context should not be shared, especially they can be modified during the rendering. To alleviate the issue,

  1. we add a read only accessor, jinjava.getGlobalContextReadOnly(),
  2. proxy the registering functions, so that a caller does not have to access the globalContext directly.
  3. make a copy of the global context as the root of scoped context for each rendering.

Drawback is once the JinJava's renderForResult is started, updating the global content (registering new filter, etc) has no effect. It might actually be desired.

Copy link
Collaborator

@mattcoley mattcoley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, can we branch deploy the renderer and run a diff test to see if this causes any issues?

Copy link
Contributor

@boulter boulter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure you can dynamically register new functions or filters. I believe that's why the list of disabled features had to be implemented separately.

@@ -123,6 +127,10 @@ public Context getGlobalContext() {
return globalContext;
}

public Context getGlobalContextReadOnly() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public Context getGlobalContextReadOnly() {
public Context getGlobalContextCopy() {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used to use getGlobalContextCopy, thought the readOnly one is better :).

@Joeoh
Copy link
Contributor

Joeoh commented May 29, 2020

Thanks for this. Looks like it will resolve the issue I introduced here. Was that your intention or did I just get lucky here?

I was also going to introduce a flag on Context that would identify it as the global. Not sure if you might consider that useful here so we can easily avoid adding to the context if we know its the global one.

@Joeoh
Copy link
Contributor

Joeoh commented May 29, 2020

Created PR to add global context flag to this: #448

@Joeoh
Copy link
Contributor

Joeoh commented Jun 3, 2020

Do you plan on merging this soon?

@hs-lsong
Copy link
Collaborator Author

hs-lsong commented Jun 3, 2020

The branch deploy was good. Planing to merge early next week. I wanted to do a bit more debugging before we stop sharing the global context.

@Joeoh
Copy link
Contributor

Joeoh commented Jun 4, 2020

Sounds good! I'll wait for your changes before merging mine. They give me an extra layer of safety for my changes.

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 this pull request may close these issues.

None yet

4 participants