Skip to content

fix: provide error instead of nil panic when cache_zone is missing#10138

Merged
juststillthinking merged 10 commits into
apache:masterfrom
Revolyssup:revolyssup/9957
Sep 13, 2023
Merged

fix: provide error instead of nil panic when cache_zone is missing#10138
juststillthinking merged 10 commits into
apache:masterfrom
Revolyssup:revolyssup/9957

Conversation

@Revolyssup

@Revolyssup Revolyssup commented Sep 1, 2023

Copy link
Copy Markdown
Contributor

Description

Fixes #9957

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Comment thread apisix/plugins/proxy-cache/memory.lua
@juststillthinking juststillthinking added the wait for update wait for the author's response in this issue/PR label Sep 5, 2023
Comment thread t/plugin/proxy-cache/memory.t Outdated
Comment thread t/plugin/proxy-cache/memory.t Outdated
Signed-off-by: Ashish Tiwari <[email protected]>
Signed-off-by: Ashish Tiwari <[email protected]>
Signed-off-by: Ashish Tiwari <[email protected]>
@Revolyssup Revolyssup removed the wait for update wait for the author's response in this issue/PR label Sep 5, 2023
kayx23
kayx23 previously approved these changes Sep 5, 2023
Comment thread t/plugin/proxy-cache/memory.t Outdated
enhance test description

Co-authored-by: Abhishek Choudhary <[email protected]>
kayx23
kayx23 previously approved these changes Sep 6, 2023
Comment thread apisix/plugins/proxy-cache/init.lua Outdated

-- For memory based cache, the default cache_zone cannot be used.
-- cache_zone will also be set as default value in case when passed empty.
if conf.cache_strategy == STRATEGY_MEMORY and conf.cache_zone == DEFAULT_CACHE_ZONE then

@juststillthinking juststillthinking Sep 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's not very reliable to judge by cache zone name, i think you could modified like this:

if local_conf.apisix.proxy_cache then
    for _, cache in ipairs(local_conf.apisix.proxy_cache.zones) do
        if cache.name == conf.cache_zone then
            if conf.cache_strategy == STRATEGY_MEMORY and not cache.disk_path then
                found = true
                break
            elseif conf.cache_strategy == STRATEGY_DISK and cache.disk_path then
                found = true
                break
            end
        end
    end
    if found == false then
        return false, "cache_zone " .. conf.cache_zone .. " not found"
    end
end

@juststillthinking juststillthinking added the wait for update wait for the author's response in this issue/PR label Sep 12, 2023
Signed-off-by: Ashish Tiwari <[email protected]>
Signed-off-by: Ashish Tiwari <[email protected]>
@Revolyssup Revolyssup removed the wait for update wait for the author's response in this issue/PR label Sep 13, 2023
@Revolyssup

Copy link
Copy Markdown
Contributor Author

@monkeyDluffy6017 I added a slightly modified version of your suggestion with comments. Please review.

@juststillthinking
juststillthinking merged commit 67057b8 into apache:master Sep 13, 2023
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.

bug: runtime error with proxy-cache plugin when using in memory caching without specifying the memory_cache zone

6 participants