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

Dont modify redis read only lua tables #14

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

hrsantiago
Copy link
Contributor

@hrsantiago hrsantiago commented Jun 11, 2022

It seems that Redis after version 6.2.7 doesn't allow us to modify global lua tables.
Probably added here: redis/redis#10651

The following error happens:

2022/06/10 22:17:11 [error] 108#0: *51 [lua] qless.lua:343: call(): ERR user_script:27: Attempt to modify a read only table script: dbf8cd1bf81f9d8ad0ae8aa84860da676603dda4, on @user_script:27., context: ngx.timer
2022/06/10 22:17:11 [error] 108#0: *51 [lua] qless.lua:343: deregister_workers(): ERR user_script:27: Attempt to modify a readonly table script: dbf8cd1bf81f9d8ad0ae8aa84860da676603dda4, on @user_script:27., context: ngx.timer

The line 27 is changing the table "table".
However it was required to make the function local to make the error stop.
It seems that this won't break for previous of Redis.

A new tag at luarocks would be appreciated, so Dockerfiles can run properly again.
https://luarocks.org/modules/pintsized/lua-resty-qless

Thanks!

@oranagra
Copy link

For the record, that change in Redis Lua engine also exists in Redis 6.2.7 (not only 7.0.0)

@pintsized
Copy link
Member

Ok thanks! Yeah, that makes sense. I'll review properly shortly.

@AdilsonDev AdilsonDev mentioned this pull request Jul 6, 2022
@pintsized pintsized merged commit 3a548a3 into ledgetech:master Jul 8, 2022
@pintsized
Copy link
Member

Merged, and a new rock published to luarocks: https://luarocks.org/modules/pintsized/lua-resty-qless/0.12-0

Really sorry for the delay!

pedropb added a commit to Shopify/qless that referenced this pull request Mar 1, 2023
More context:
- [qless fix](ledgetech/lua-resty-qless#14)
- [redis 6.2.7 breaking
  change](redis/redis#10651)

I did not regenerate the files from qless-core using the `make`
instructions from the README.md because the commit we have been using
[^1] dates back to Aug 2017, and the same fix has not been applied to
seomoz/qless-core yet. See [^2]

[1]:
seomoz/qless-core@36199bf

[2]: seomoz/qless-core#90
pedropb added a commit to Shopify/qless that referenced this pull request Mar 1, 2023
More context:
- [qless fix](ledgetech/lua-resty-qless#14)
- [redis 6.2.7 breaking
  change](redis/redis#10651)

I did not regenerate the files from qless-core using the `make`
instructions from the README.md because the commit we have been using
[^1] dates back to Aug 2017, and the same fix has not been applied to
seomoz/qless-core yet. See [^2]

[^1]:
seomoz/qless-core@36199bf

[^2]: seomoz/qless-core#90
pedropb added a commit to Shopify/qless that referenced this pull request Mar 1, 2023
More context:
- [qless fix](ledgetech/lua-resty-qless#14)
- [redis 6.2.7 breaking
  change](redis/redis#10651)

I did not regenerate the files from qless-core using the `make`
instructions from the README.md because the commit we have been using
[^1] dates back to Aug 2017, and the same fix has not been applied to
seomoz/qless-core yet. See [^2]

[^1]: seomoz/qless-core@36199bf

[^2]: seomoz/qless-core#90
pedropb added a commit to Shopify/qless that referenced this pull request Mar 1, 2023
More context:
- [qless fix](ledgetech/lua-resty-qless#14)
- [redis 6.2.7 breaking
  change](redis/redis#10651)

I did not regenerate the files from qless-core using the `make`
instructions from the README.md because the commit we have been using
[^1] dates back to Aug 2017, and the same fix has not been applied to
seomoz/qless-core yet. See [^2]

[^1]: seomoz/qless-core@36199bf

[^2]: seomoz/qless-core#90
bak1an added a commit to libqless/qless-core that referenced this pull request Oct 30, 2024
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.

3 participants