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

Unable to cache a value of null #824

Closed
4 tasks done
Telokis opened this issue Oct 12, 2020 · 11 comments
Closed
4 tasks done

Unable to cache a value of null #824

Telokis opened this issue Oct 12, 2020 · 11 comments
Milestone

Comments

@Telokis
Copy link

Telokis commented Oct 12, 2020

Prerequisites

Please answer the following questions for yourself before submitting an issue.

  • I am running the latest version
  • I checked the documentation and found no answer
  • I checked to make sure that this issue has not already been filed
  • I'm reporting the issue to the correct repository

Current Behavior

null and undefined cannot be cached by moleculer.

Expected Behavior

I expected at least null to be a cacheable value.

Failure Information

More often than not, a specific action will be invalid. For example, trying to get a specific id from database but it doesn't exist.
In such a case, it makes sense to return null but I don't want to be flooded by requests since I know it won't return anything other than null until I ask the cache to be cleaned.

Steps to Reproduce

  1. Create a service containing an action wich returns null.
  2. Enable caching for the action.
  3. Add a sleep in order to voluntarily slow the action.
  4. Try to call the action, notice it takes time.
  5. Try to call the action again, the caching didn't occur and it still takes time.

Reproduce code snippet

const broker = new ServiceBroker({
    logger: console,
    transporter: "NATS",
    cacher: "Memory"
});

broker.createService({
    name: "test",
    actions: {
        example: {
            cache: true, // Enable caching
            async handler(ctx) {
                // Artificial pause for demonstration purposes
                await new Promise((resolve) => setTimeout(resolve, 1000));

                return null; // Return null
            }
        }
    }
});

Possible solution

Instead of using a weak comparison against null in the cacher base (cachers/base.js#L303) we can strongly compare against undefined using !== undefined.
But I think the best solution would be to use a Symbol which is specific to moleculer. This way, we know nobody will be tempted to return it from inside their action.

However, I understand that this would be a breaking change (and I'm a bit saddened by this fact).

@icebob
Copy link
Member

icebob commented Oct 13, 2020

Could you create a PR? I think we can avoid the breaking change if we switch the old/new logic with a cacher option, e.g. storeNullValues: true

@icebob
Copy link
Member

icebob commented Oct 13, 2020

By the way, as I see, the value is stored, just not return it, right?

@Telokis
Copy link
Author

Telokis commented Oct 13, 2020

By the way, as I see, the value is stored, just not return it, right?

Yes, from what I noticed, the value is properly stored and retrieved but the code I highlighted makes it seem like a cache miss and calls the action anyway.

Regarding the PR, I could do it if we come up with a good non-breaking solution, yes.

I thought it could be regarded as a breaking change for people who use custom cachers but I guess the configuration option fixes this issue.

@icebob
Copy link
Member

icebob commented Oct 13, 2020

Yeah, but in this case, the option name enableNullValues would be better because the storing is good. And as you mentioned, we should return a Symbol in get if it's not found in the cache and also check this symbol in the base middleware.

@Telokis
Copy link
Author

Telokis commented Oct 13, 2020

Thanks for your answer. To make sure I understand:

  • We add a configuration option enableNullValues to cachers/base.js#L29
  • If enableNullValues is set to true
    • [Cacher] Return a specific Symbol instead of null to represent a cache miss.
    • [Middleware] When checking if a cache lookup missed, we check against the Symbol.
  • If enableNullValues is false or undefined
    • [Cacher] We return null as we do now.
    • [Middleware] We weakly check against null to detect cache misses.

Is this correct?

@icebob
Copy link
Member

icebob commented Oct 16, 2020

Yeah, you are correct. Could you make a PR? Of course, need to add relevant tests, as well.

@intech
Copy link
Member

intech commented Feb 26, 2021

@Telokis any news?

@Telokis
Copy link
Author

Telokis commented Feb 26, 2021

@intech Hi! No, I didn't work on/with Moleculer since around then. I still need to properly finish my PR #829 and won't consider tackling this issue until that PR is done.

@intech
Copy link
Member

intech commented Feb 26, 2021

@intech Hi! No, I didn't work on/with Moleculer since around then. I still need to properly finish my PR #829 and won't consider tackling this issue until that PR is done.

@Telokis thanks for the answer. I have now submitted for consideration changes to the serialization algorithm in order to solve 3 urgent problems at once, including yours.
Therefore, I wanted to warn you, before you start work, let me know in the comments about it. Perhaps we will already have another solution ready.

@Telokis
Copy link
Author

Telokis commented Feb 26, 2021

@intech Hi! No, I didn't work on/with Moleculer since around then. I still need to properly finish my PR #829 and won't consider tackling this issue until that PR is done.

@Telokis thanks for the answer. I have now submitted for consideration changes to the serialization algorithm in order to solve 3 urgent problems at once, including yours.
Therefore, I wanted to warn you, before you start work, let me know in the comments about it. Perhaps we will already have another solution ready.

@intech Ok, seems like a good idea, thank you for letting me know about it!

@icebob icebob added this to the 0.15 milestone Jan 14, 2023
@icebob
Copy link
Member

icebob commented Jan 14, 2023

I've added a missingResponse cacher option. The cachers return the value of it if the key is not found. By default, the value is undefined, so it enables to store null values, as well. If you would like to store undefined values, then you can change it to a symbol, e.g.: missingResponse: Symbol("MISSING").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants