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

JDK16 Support #4785

Merged
merged 2 commits into from
May 10, 2021
Merged

JDK16 Support #4785

merged 2 commits into from
May 10, 2021

Conversation

jansupol
Copy link
Contributor

Signed-off-by: jansupol [email protected]

Signed-off-by: jansupol <[email protected]>
Copy link
Member

@jbescos jbescos left a comment

Choose a reason for hiding this comment

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

I wrote a small comment to double check.


/* package */ Cache<Class<?>, Boolean> getJaxRsResourceCache() {
/* CDI injection hack */
if (jaxRsResourceCache == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we synchronize this or are we sure only one thread can invoke it the first time?. Something like:

    /* package */ Cache<Class<?>, Boolean> getJaxRsResourceCache() {
        /* CDI injection hack */
        if (jaxRsResourceCache == null) {
           synchronize(this) {
                if (jaxRsResourceCache == null) {
                      jaxRsResourceCache = new Cache<>(clazz -> Resource.from(clazz) != null);
                }
           }
        }
        return jaxRsResourceCache;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. But invoking this multiple times would not break anything, there would be two instances of the cache & Resource.from would be invoked twice instead of having it used from the cache. I am not sure the double-checked locking is worth it in this case.

Copy link
Member

@jbescos jbescos May 7, 2021

Choose a reason for hiding this comment

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

I don't know all the details about how is going to be used, but imagine that there is a singleton that invokes that method and it saves it as a field. So next time it wants to use the cache, it reads it from the field (instead of getting it from getJaxRsResourceCache()).

It will not harm to have that lock because it will only lock at the beginning and we get rid of unnecessary extra memory usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It happens that all the if (jaxRsResourceCache == null) { block is unnecessary, so we could have dropped it

@jansupol jansupol requested a review from jbescos May 6, 2021 19:52
@jansupol jansupol merged commit e129ced into eclipse-ee4j:master May 10, 2021
@jansupol jansupol deleted the jdk16 branch May 10, 2021 11:43
@jansupol jansupol self-assigned this May 10, 2021
@jansupol jansupol added this to the 2.35 milestone May 10, 2021
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

3 participants