ComputerCraft icon indicating copy to clipboard operation
ComputerCraft copied to clipboard

Replace LuaJ with Cobalt

Open SquidDev opened this issue 7 years ago • 30 comments

It does not add any new dependencies for compiling, running or using the mod.

I realise this is a bit of a long shot as it fails the first criteria, but I do still think this is worth considering.

Cobalt is a fork of LuaJ I started a long time ago to address a couple of bugs (#91, #97) in LuaJ. It's kinda expanded a bit since then, with the following improvements:

  • Many bug fixes - Cobalt passes a significantly larger proportion of the Lua tests than normal LuaJ. This includes fixes to string formatting, weak metatables and non-string errors (#91, #97, #333, #425, #592).
  • No global state: - Unlike LuaJ, Cobalt does not have any global state. This means the string metatable no longer needs to be sandboxed. It also would allow for multi-threading computers, and (maybe) exposing the debug API without any issues.
  • Improved support for yield prevention (solves issues like #431).
  • Improved error messages (pront() would give you attempt to call global 'pront' (a nil value) instead, loadstring doesn't include the caller, etc...).

However, this is nowhere near as used as LuaJ, so there may be some unknown issues. There is also no Lua 5.2/5.3 version, which may disrupt your future plans.

Synopsis changes in this PR

  • Replace LuaJMachine with CobaltMachine
  • Remove sandboxing of string metatable and environment.
  • Change the timeout system to run on instructions instead of API calls. One issue with the original system was it didn't trigger in simple infinite loops such as while true do end. With the new system, you will get an error mid iteration. It should also fire errors when in very long pattern matches.

Potential improvements

  • Allow running computers across multiple threads. CCTweaks includes a system for multi-threading computers, but it requires more thorough testing - there are still issues with deadlocks involving peripheral methods.
  • Inject the debug API into the environment. The debug API is immensely useful, so would be nice to have. However, there may be some way I am unaware of to abuse it - even if you cannot leak into other computers any more.
  • Inject require and the package library. Cobalt adds the ability to use custom filesystem providers, so it should be possible to add require without breaking the sandbox.
  • Expose more of the os library - As mentioned above, Cobalt allows custom filesystem providers, meaning os.rename and os.remove can be implemented in a sandbox friendly manner.

Potential issues

  • I have tried to strike a balance between sticking as close to normal Lua as possible, whilst still maintaining comparability with LuaJ. I'm not aware of any issues Cobalt has caused (it is used on several CC servers without a problem), but there may be some.
  • As mentioned above, while Cobalt has been tested, it isn't as widely used as LuaJ, so there may be some hidden bugs.

Note: I haven't deleted any of the LuaJ sources in this PR, as to keep it readable. That will need to be done in a later commit.

SquidDev avatar May 01 '17 18:05 SquidDev

This would also make it easier to distribute CC, and make it easier for other developers to compile against CC. With cobalt properly registered as a Gradle dependency, you no longer need a set of scripts to deploy and instead can compile a fully functional version of CC with just ./gradlew build.

dmarcuse avatar May 01 '17 18:05 dmarcuse

Since this is quite a monster of a PR, I think it's worth emphasising that Cobalt has been tested in battle quite extensively before, so merging immature code should not be a concern here. Notably, SwitchCraft, when it was still active, had this fork in use via CCTweaks, which allowed it to stabilise significantly.

Lignum avatar May 01 '17 18:05 Lignum

All I can say is :tada: :+1: :sparkles: :clap: :package: :shipit: :ok_hand: :wine_glass: :repeat:, please merge.

viluon avatar May 01 '17 21:05 viluon

@jamierocks I've deliberately avoided that in order to reduce noise and make it clear what changes this PR makes. It should be trivial to delete the LuaJ sources at a later date. I've removed the code which will package LuaJ, so at least it's not shipping two Lua versions 😄.

SquidDev avatar May 03 '17 20:05 SquidDev

@SquidDev I'd still say remove it tbh. Either way at least remove libs/luaj.*.jar

jamierocks avatar May 03 '17 20:05 jamierocks

(I know this is not the best place to mention it but) have you considered looking into merging your fixes into LuaJ itself? would befit more than just the CC community.

simon816 avatar May 04 '17 02:05 simon816

+1. How much code will this break?

Restioson avatar May 04 '17 05:05 Restioson

+1. How much code will this break?

If I've done my job correctly, none. Several servers use Cobalt already, and haven't experienced any issues. It is a fork of LuaJ, and so shares most of its functionality.

Have you considered looking into merging your fixes into LuaJ itself? would befit more than just the CC community.

I haven't really thought about it. AFAIK LuaJ 2 (the version CC uses) is no longer developed. Many of the changes also break binary compatibility - hence the change in package name. I'd love for Cobalt to be merged into LuaJ, I just don't think it is feasible given some of the changes made.

SquidDev avatar May 04 '17 07:05 SquidDev

Nice! That is very good. Will any java code break, or are the changes easily fixable?

Restioson avatar May 04 '17 15:05 Restioson

No code using the public API should break (IPeripheral, ILuaObject, etc...). To be accessing the Lua VM you'd have to be using all sorts of reflection trickery - Dan has said that he isn't fussed about that.

SquidDev avatar May 04 '17 15:05 SquidDev

That's cool. +1, definitely merge this

Restioson avatar May 04 '17 15:05 Restioson

@dan200 I realise there are other PRs out there which are objectively more important than this one, but I was wondering if you have any thoughts on this PR? I realise this is a massive change so isn't going to be an instant merge (or a merge at all for that matter), but I would be interested to know your thoughts.

SquidDev avatar May 06 '17 23:05 SquidDev

Not trying to rush things, but it would be really nice to see some feedback on this @dan200. It would be a great change and fix a lot of headache with LuaJ, and open up a lot of new possibilities for features.

dmarcuse avatar May 08 '17 17:05 dmarcuse

If you're planning to change the Lua engine, why not use LuaJIT directly? It's extremely fast and supports most architectures. I understand it's outside of the JVM and so would require additional binding work, but that's something that could be worth it.

JustPingo avatar May 28 '17 18:05 JustPingo

@JustPingo Cobalt is pretty much LuaJ with some bug fixes and minor improvements, meaning it isn't as dramatic a change as swapping to an alternative Lua runtime (such as LuaJIT or PUC Lua).

On the subject of LuaJIT itself, I can see a couple of potential issues:

  • In tight loops (such as while true do end), debug hooks are not run. You'd probably have to disable the JIT to prevent this (though the interpreter's performance is still incredible).

  • LuaJ's semantics don't entirely match up with PUC Lua/LuaJIT's. This is entirely LuaJ's fault, but many CC programs rely on its "broken" functionality. I don't know how much this matters, but worth bearing in mind.

  • ILuaAccess.yield and friends rely on Lua coroutines being backed by Java threads. This doesn't quite work with the coroutine implementation of normal Lua. It is possible to write an adaptor (which is what I've done for Rembulan), but the performance isn't great.

    I'd really like to rewrite how yields are implemented, but that would have to wait for a new Minecraft version to avoid breaking binary compatibility - I've got a mostly-working implementation of "true" coroutines for Cobalt but, as mentioned previously, it won't play nice with CC's current implementation.

I have got a WIP Lua to Java JIT which, if I could optimise it a little more, could be worth looking into. When Java 9 ships, one could also look into writing a Graal/Truffle Lua implementation. I might be over complicating this a little though 😄.

SquidDev avatar May 28 '17 18:05 SquidDev

What kind of broken functionality are we dependent on? If its a small thing it might make sense to drop it?

Wojbie avatar May 28 '17 18:05 Wojbie

@Wojbie The thing which first springs to mind is that error and stack levels function a little differently. A common pattern in CC is to use pcall(error, "", i) to get a traceback - this doesn't work in PUC Lua. Though if #216 gets implemented this isn't really a problem any more.

One thing worth noting is that using a Java implementation of Lua means we can execute bytecode without a security risk - allowing for programs like JVML-JIT and LuaLua. Sure, this is a small price to pay, but it would be a shame to lose this sort of functionality.

SquidDev avatar May 28 '17 20:05 SquidDev

@SquidDev

  • Is this relevant? I mean, that's not a major feature nor an irreplaceable one. And as you say, the interpreter still is astonishing compared to LuaJ.
  • Now I guess that's a matter of personal philosophy, but I feel like relying on inaccurate behaviours is pretty bad globally, and thus dropping compatibility for minor stuff seems reasonable to me. Someone new learning Lua through the doc could be confused by those problems, and an experienced Lua programmer might also be confused about some of those bugs they would encounter.
  • I don't understand what you mean. Aren't native Lua coroutines doing everything that is needed?

I don't understand the theoretical risks of using Lua outside of the JVM. OpenComputers does it and disables bytecode as well. Why? I understand some exploit could end up with memory corruption, but couldn't that just happen in Java as well?

Now I totally agree with replacing LuaJ with Cobalt. I'm just considering how eventually it could be replaced by LuaJIT, which is still clearly superior.

JustPingo avatar May 28 '17 21:05 JustPingo

@JustPingo

I don't understand what you mean. Aren't native Lua coroutines doing everything that is needed?

Yes. It's more about how ComputerCraft currently interacts with coroutines and expects them to behave. Methods like ILuaAccess.yield block the current thread until the coroutine is resumed again. As "true" coroutines don't work the same way, you have to call each CC method in a separate thread, translating the blocking ILuaAccess.yield calls into the appropriate coroutine methods. It's possible, just a bit of a faff and rather inefficient.

I agree that the none of these points are major, but I do still think they are worth bearing in mind. They aren't deterring factors, but should be considered if Dan chooses to use LuaJIT.

The broken behaviour relating to pcall is actually required in order to replicate missing functionality, so you can't blame people for depending on it - though as mentioned, it won't be needed if debug.traceback is added so I don't think it is worth worrying about.

I also maintain that using LuaJIT is a totally separate issue - this is just minor modifications to LuaJ, LuaJIT would require a rewrite of much of ComputerCraft's internals.

SquidDev avatar May 28 '17 21:05 SquidDev

Please merge

ghost avatar Jul 14 '17 20:07 ghost

Hate me for reviving this, but why is this stagnant...? I would've assumed this'd be the first PR to get into CC, but it's still sitting here open. Like is there some hitch with this? It doesn't even seem like a dual-edge sword, like most PR's that stay open for as long as this one are...

hugeblank avatar Oct 01 '17 21:10 hugeblank

I've just finished doing a tiny bit of work on Cobalt with some optimising and bug fixing. As I'm preparing for a release, I'm wondering whether it would be better to reference Cobalt as a git submodule instead of a maven artifact.

If Cobalt were a submodule, one wouldn't have to depend on an external maven server. Whilst Bintray provides one for free, it may be something we want to avoid. Submodules also simplify updating, as one doesn't need to bother with the whole "bump version, upload, update dependents" faff.

However, there is a downside of this: git submodules. These introduce all sorts of additional complications with cloning and pulling which we may want to avoid.

I'd really appreciate other people's thoughts on the matter - I'm not sure what the best course of action here is.

SquidDev avatar Oct 05 '17 19:10 SquidDev

Most IDEs nowadays handle submodules very well, including auto-updating them on pull and pushing changes to them (if changes are done).

Vexatos avatar Oct 05 '17 19:10 Vexatos

You could just copy the source to the CC repo directly, rather than a submodule, akin to how LuaJ is currently set up, but using a submodule or maven artifact would be much better.

dmarcuse avatar Oct 05 '17 20:10 dmarcuse

I don't really see a problem with making Cobalt a submodule. How does it complicate pulling and cloning? Sorry if it's obvious to others, I don't frequently work with submodules myself. I presume that you only need to use a specific command when updating them, however.

viluon avatar Oct 06 '17 20:10 viluon

@viluon There's lots of small things, like having to use git clone --recursive instead of git clone. I believe there are also complications involved with pulling also updating the submodule when you don't want it to. None of these are major issues, but they complicate an already confusing system.

SquidDev avatar Oct 06 '17 23:10 SquidDev

Frankly, I see little benefit to using a submodule. ComputerCraft, will always want to be using release code - little bugs, more accountability (I see people update submodules all the time carelessly).

jamierocks avatar Oct 06 '17 23:10 jamierocks

@jamierocks the submodule can track a release branch, and frankly, tracking master isn't a problem with the right branching model.

@SquidDev can't you clone the submodule on build?

viluon avatar Oct 07 '17 15:10 viluon

@viluon clearly, I need to stop posting late at night - I did not articulate my point very well at all.

I was trying to get across that it would put more emphasis on ComputerCraft devs to know what is going on with Cobalt development - which would possibly result in Cobalt not getting updated. (this could be because not sure which releases/commits are going to break CC - appropriately versioned dep releases doesn't have this problem, although there quick fixes can't be merged quickly)

My other point, was that tracking a specific version is just a much more painful method than using a Maven dependency (having to recursively clone, etc) - the two get the exact same result, the easiest option IMO would be the desired one.

I totally see your point with using a branching model (that has a 'stable' branch), however in that case, Cobalt would have to make alterations - for something which can already be accomplished (through Maven dep).

Basically submodules are only useful if you know exactly what development is going on upstream. If you are tracking specific releases, you are just making life harder - having to combine the build scripts of Cobalt into CC, etc.

Don't get me wrong - I love submodules, however from my use of them, I have found that Maven dependencies (when they can be used) are far more appropriate - especially when you'd have to combine two builds. (although at least both use Gradle here)

jamierocks avatar Oct 07 '17 16:10 jamierocks

It's been a year, so I thought it would be a good time to mention that we've been running this on several relatively popular CC-based servers for a while now, and not experienced any issues.

I can't comment on the benefits it has brought, as they're all relatively subtle and so one doesn't tend to notice it. That being said, there's been many times the additional information provided in errors has come in useful, as have the improvements to yield protection.

Several servers have also been running with Cobalt's debug API enabled (which is not exploitable in the same way LuaJ's implementation is). We haven't seen any exploits, and the built-in implementation of debug.traceback has been especially useful.

Anyway, happy birthday to ComputerCraft being open sourced 🍰. Here's to year 2 🍻. Thank you to everyone who has contributed!

SquidDev avatar May 01 '18 18:05 SquidDev