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

API Redesign #171

Merged
merged 23 commits into from
May 24, 2024
Merged

API Redesign #171

merged 23 commits into from
May 24, 2024

Conversation

gudzpoz
Copy link
Owner

@gudzpoz gudzpoz commented Apr 30, 2024

closes #169

Planned Changes

  • Throw Lua-side errors as Java exceptions.
    • Decide whether they should be checked exceptions.
      Probably not I guess?
  • LuaValue improvements.
    • Map interface implementation for tables.
    • toProxy().
    • Conversions and calls.
    • Remove the deprecated LuaValue#close and delegate the job fully to cleaners/finalizers.
    • Add LuaFunction to simplify JFunction usages.
    • Rename LuaThread::execute to eval to match JSR223 naming.
  • Lua interface improvements:
    • Global value manipulation API (implementing a LuaThread interface, similar to a LuaValue table).
  • LuaNative clean up.
    • Check LuaJNatives for more useless functions present in the LuaNative interface.
      • Some Lua C API functions are common between Lua versions, for example, luaL_gsub. Our bindings are mostly programmatically generated and include these functions. However, luaL_gsub is just doing C string substitution (no, it does not involve any Lua) and should be safe to remove.
      • luaL_gsub.
      • luaJ_pcall.
      • luaJ_metatable.
    • Make C APIs in LuaNatives public.
  • Rename java.catched to java.caught.
  • Improve test coverage (a 96% should be good to go).
  • Update documentation.
  • [-] Release.

Non-API Changes

  • Upgrade Gradle & Android Gradle Plugin to allow desugaring and using Java 8 features in tests.
  • Fix LuaJ coroutine deadlocks.

@gudzpoz
Copy link
Owner Author

gudzpoz commented Apr 30, 2024

After adding more tests, LuaJ's approach to coroutines (by using OS threads) turns out to be problematic: it can deadlock.

In PUC-Rio Lua, all operations are executed a single thread:

synchronized @<thread-1> {
  lua calls another coroutine {
    lua access proxy objects {
      synchronized @<thread-1> { // luajava proxies synchronizes to ensure thread safety
        operations can be done since it it fine to nest sync-scopes in the same thread
      }
    }
  }
}

However, LuaJ uses OS threads for coroutines:

synchronized @<thread-1> {
  lua calls another coroutine {
    here is @<thread-2> !!!
    lua access proxy objects {
      synchronized @<thread-2> { // deadlock here
        unreachable~
      }
    }
  }
}

However, after looking into the implementation of LuaThread in LuaJ, it actually seems possible to re-implement coroutines with a single-threaded executor. No, it's not.

Fixed in 46ea2c3 (#171) and a24b929 (#171) .

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2024

Codecov Report

Attention: Patch coverage is 99.69605% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 96.55%. Comparing base (f6872e0) to head (09596f4).
Report is 1 commits behind head on main.

Files Patch % Lines
...in/java/party/iroiro/luajava/luaj/LuaJNatives.java 90.90% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #171      +/-   ##
============================================
+ Coverage     92.73%   96.55%   +3.81%     
- Complexity      847      912      +65     
============================================
  Files            43       44       +1     
  Lines          2368     2465      +97     
  Branches        281      286       +5     
============================================
+ Hits           2196     2380     +184     
+ Misses          125       61      -64     
+ Partials         47       24      -23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gudzpoz added 11 commits May 23, 2024 20:33
And fix bugs found in LuaTableValue
Originally {@link #recycleReferences} was synchronized on mainThread.
However, the synchronization is deemed unnecessary and was later removed.

1. Only {@link #checkStack} and {@link #gc} calls recycleReferences.
2. We ask the user to synchronize on mainThread whenever they try to use LuaJava
   in a multi-thread application.
3. {@link #gc} is only called by the user and should already under a synchronization block.
4. {@link #checkStack} is used by lots of member functions in AbstractLua, but:
   - When it is called by the user, the user should do the synchronization instead.
   - When it is called from the Lua side via {@link JuaAPI}, either:
     - The user is calling, for example, {@link #run}, and synchronizes already on mainThread;
     - The call comes from a Java proxy object, which also synchronizes on mainThread.

Notably, adding a synchronization block will make LuaJ coroutines malfunction:
- The user synchronized on mainThread;
  - The user then called {@link #run} to execute a snippet to call a coroutine;
    - LuaJ created a Java thread for the coroutine;
      - Now, if the coroutine ever calls {@link #recycleReferences}
        (with, e.g., any stack operation), the two threads now deadlocks.
The removal of the synchronization block should fix the issue above.
Handles the case when a proxy object is used on the Lua side.
- Remove duplicates of lua_pcall functions
- Remove luaL_gsub
- Also fix AndroidScriptTest
@gudzpoz gudzpoz marked this pull request as ready for review May 24, 2024 13:16
@gudzpoz gudzpoz merged commit fc7f173 into main May 24, 2024
13 checks passed
@gudzpoz gudzpoz deleted the improve-api branch May 24, 2024 14:19
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.

API Redesign
2 participants