Unable to cache a value of null

Telokis commented 3 years ago

Prerequisites

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

[x] I am running the latest version
[x] I checked the documentation and found no answer
[x] I checked to make sure that this issue has not already been filed
[x] 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

Create a service containing an action wich returns null.
Enable caching for the action.
Add a sleep in order to voluntarily slow the action.
Try to call the action, notice it takes time.
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 commented 3 years ago

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 commented 3 years ago

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

Telokis commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

intech commented 3 years ago

@Telokis any news?

Telokis commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 1 year ago

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").

moleculerjs / moleculer