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

THRIFT-4386 Add Lua 5.3/5.4 support #3012

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

thomasbruggink
Copy link
Contributor

@thomasbruggink thomasbruggink commented Jul 28, 2024

This PR adds support for running with Lua 5.3 and 5.4 while also keeping backwards compatibility with Lua 5.1.
Updated ubuntu-jammy dockerfile to install Lua 5.4 for testing.

Fixed cross-testing in a follow up PR to confirm this works with for both Lua versions with the Java client.

This PR adds support for running with Lua 5.3 and 5.4 while also keeping
backwards compatibility with Lua 5.2.
Updated ubuntu-jammy dockerfile to install Lua 5.4 for testing.
@Jens-G Jens-G added the lua label Jul 29, 2024
@thomasbruggink
Copy link
Contributor Author

@Jens-G do you know anyone who might be able to review this?

If there is currently no active Lua maintainer would it also be acceptable to have the cross tests pass to proof the existing implementation works as expected? If yes I'll move the cross test fixes into this PR :)

@Jens-G
Copy link
Member

Jens-G commented Aug 25, 2024

+1 agreed

This PR adds uuid support through a custom TUUID class.
Fixed the cross test to confirm the implementation works by
cross-testing against Java.

Implementation tested on both Lua 5.2 and 5.4.
@thomasbruggink
Copy link
Contributor Author

Added 4 more commits to ensure cross testing works as expected.
UUID tests are required to confirm anything so I added THRIFT-5804 to this PR as well.

The follow cross tests now work:

  • lua client -> lua server (100%)
  • java client -> lua server (100%)
  • netstd -> lua server (66%)

The failing netstd tests are the result of 15mb Json data which also times out on netstd client -> netstd server.

Note that to get Http working I needed to slightly modify the oneway implementation in the compiler and added a new flushOneway method. There is a default NOOP handler in TTransportBase which ensures that custom implementations of TTransportBase will keep working, but users that upgrade to the thrift version that contains this commit will also need to update their TTransportBase.lua.

function TUUIDfromString(str)
local iterator = string.gmatch(str, "[A-Fa-f0-9][A-Fa-f0-9][A-Fa-f0-9][A-Fa-f0-9]")
return TUUID:new {
zero = libluabitwise.buor(libluabitwise.ushiftl(tonumber(iterator(), 16), 16), tonumber(iterator(), 16)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note that bitshifting was added in Lua 5.3, so if support for Lua 5.2 is dropped this implementation and the entire libluabitwise C module can be deleted.

The current usocket implementation drops packages when `send()` returns
`EGAIN` or a smaller size than was expected to be send.
This implementation will retry up to `SEND_RETRY_COUNT` (5) times before
giving up with the original error code.
When data was send partially the new implementation tries again with the
remaining data.

This can happen we sending large chunks of data over a non blocking
socket. The error could be reproduced when using the 15mb binary test
from netstd.
The Lua HttpTransport didnt close the Http connection properly resulting
in an abruptly closed connection.
Oneway methods are expected to return before entering the implementation
method. Since Lua has no concept of threading the method will always
block and cross testing will fail.

This implementation adds a new `flushOneway` method to `TTransportBase`
which ensures that transports like Http can submit closing responses before
entering the oneway method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants