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

Cache clearing enhancement (clear additional cache, synchronization fix): #77

Conversation

JCgH4164838Gh792C124B5
Copy link
Contributor

Attempt to resolve issue #34:

  • Add a clearAdditionalCache() method to clear some additional cache elements.
    Using a separate method as changing the behaviour of clearCache() could negatively impact existing usage.
  • Added missing synchronization within clearCache() for many elements that should have synchronization for modifications.

- Add a clearAdditionalCache() method to clear some additional cache
elements.
Using a separate method as changing the behaviour of clearCache() could
negatively impact existing usage.
- Added missing synchronization within clearCache() for many elements
that should have synchronization for modifications.
- Added guard to avoid error output for matching score methods where one is
abstract and the other concrete.  Either method works when called, so the
error output isn't needed for that specific condition (confirmed via manual
debugging to force usage of both abstract and concrete method call).
- Added test based on issue orphan-oss#42 which shows the syserr output is no longer
produced for this scenario.
@JCgH4164838Gh792C124B5
Copy link
Contributor Author

Hello OGNL Team.

After reviewing open issues, decided to try to replicate issue #42 and was able to confirm that it still exists. A new test confirmed the syserr output was occurring for the reported scenario but the actual call by OGNL worked just fine.

Looking at the code and "forcing both paths" (abstract and concrete) the proper result was returned in both cases. So the OgnlRuntime.findBestMethod() in this PR has now been updated with checks so that it will not output the error anymore unless both methods being compared have the same "concreteness" or "abstractness" - since that would definitely be a weird state that should be reported.

@JCgH4164838Gh792C124B5
Copy link
Contributor Author

I also encountered some weird behaviour for some of the Security Sandbox tests when performing interactive debugging for the last update to this PR.

Occasionally one of the sandbox tests TestOgnlRuntime.test_Call_Method_In_JDK_Sandbox_Privileged())
failed due to the cause being a NullPointerException instead of the expected type.

Running the test module outside of the debugger never produced failures, so it's unclear if it was just interference by the IDE debugger or some underlying condition in the logic. Just wanted to leave a note for future reference in case an issue pops up.

- Lukasz indicated he approved the JPMS package name "ognl".
- Added the Automatic-Module-Name manifest-entry for this.
@lukaszlenart
Copy link
Collaborator

@JCgH4164838Gh792C124B5 conflict :(

…calOGNL_3_1_ClearCacheEnh

# Conflicts:
#	src/java/ognl/OgnlRuntime.java

- Manually resolved merge conflict for OgnlRuntime.java
@JCgH4164838Gh792C124B5
Copy link
Contributor Author

Hello @lukaszlenart .

Manual resolution of merge conflicts completed (but might conflict if #75 is accepted/merged).
Additional commit to use source/target 1.6 was required to get Travis CI checks to complete (just like in #75).

It should be easier for others to review now, at least. 😄

@lukaszlenart
Copy link
Collaborator

LGTM 👍

@lukaszlenart lukaszlenart merged commit 74f857c into orphan-oss:ognl-3-1-x Sep 12, 2019
@JCgH4164838Gh792C124B5 JCgH4164838Gh792C124B5 deleted the localOGNL_3_1_ClearCacheEnh branch September 28, 2019 16:33
JCgH4164838Gh792C124B5 pushed a commit to JCgH4164838Gh792C124B5/ognl that referenced this pull request Sep 29, 2019
…L_3_1_ClearCacheEnh

Cache clearing enhancement (clear additional cache, synchronization fix):

(cherry picked from commit 74f857c)
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.

2 participants