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

Replace LuaJ with Cobalt #163

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
@SquidDev
Copy link
Contributor

SquidDev commented May 1, 2017

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).
  • 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.

@dmarcuse

This comment has been minimized.

Copy link
Contributor

dmarcuse commented May 1, 2017

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.

@Lignum

This comment has been minimized.

Copy link
Contributor

Lignum commented May 1, 2017

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.

@viluon

This comment has been minimized.

Copy link

viluon commented May 1, 2017

All I can say is 🎉 👍 👏 📦 :shipit: 👌 🍷 🔁, please merge.

@jamierocks
Copy link

jamierocks left a comment

Remove luaj distribution in repo

@SquidDev

This comment has been minimized.

Copy link
Contributor Author

SquidDev commented May 3, 2017

@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 😄.

@jamierocks

This comment has been minimized.

Copy link

jamierocks commented May 3, 2017

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

@simon816

This comment has been minimized.

Copy link

simon816 commented May 4, 2017

(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.

@Restioson

This comment has been minimized.

Copy link
Contributor

Restioson commented May 4, 2017

+1. How much code will this break?

@SquidDev

This comment has been minimized.

Copy link
Contributor Author

SquidDev commented May 4, 2017

+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.

@Restioson

This comment has been minimized.

Copy link
Contributor

Restioson commented May 4, 2017

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

@SquidDev

This comment has been minimized.

Copy link
Contributor Author

SquidDev commented May 4, 2017

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.

@Restioson

This comment has been minimized.

Copy link
Contributor

Restioson commented May 4, 2017

That's cool. +1, definitely merge this

@SquidDev

This comment has been minimized.

Copy link
Contributor Author

SquidDev commented May 6, 2017

@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 SquidDev force-pushed the SquidDev-CC:feature/cobalt branch from 7a83a54 to 72dab14 May 7, 2017

@dmarcuse

This comment has been minimized.

Copy link
Contributor

dmarcuse commented May 8, 2017

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.

@SquidDev SquidDev force-pushed the SquidDev-CC:feature/cobalt branch 2 times, most recently from fb51233 to b8b0496 May 13, 2017

@JustPingo

This comment has been minimized.

Copy link

JustPingo commented May 28, 2017

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.

@SquidDev

This comment has been minimized.

Copy link
Contributor Author

SquidDev commented May 28, 2017

@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 😄.

@Wojbie

This comment has been minimized.

Copy link
Contributor

Wojbie commented May 28, 2017

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

@SquidDev

This comment has been minimized.

Copy link
Contributor Author

SquidDev commented May 28, 2017

@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.

@JustPingo

This comment has been minimized.

Copy link

JustPingo commented May 28, 2017

@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.

@SquidDev

This comment has been minimized.

Copy link
Contributor Author

SquidDev commented May 28, 2017

@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.

@steamp0rt

This comment has been minimized.

Copy link

steamp0rt commented Jul 14, 2017

Please merge

@SquidDev SquidDev force-pushed the SquidDev-CC:feature/cobalt branch from c4a66d4 to 5e719a0 Sep 10, 2017

@SquidDev SquidDev force-pushed the SquidDev-CC:feature/cobalt branch from 5e719a0 to c55d431 Sep 10, 2017

@hugeblank

This comment has been minimized.

Copy link

hugeblank commented Oct 1, 2017

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...

@SquidDev

This comment has been minimized.

Copy link
Contributor Author

SquidDev commented Oct 5, 2017

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.

@Vexatos

This comment has been minimized.

Copy link

Vexatos commented Oct 5, 2017

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

@dmarcuse

This comment has been minimized.

Copy link
Contributor

dmarcuse commented Oct 5, 2017

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.

@viluon

This comment has been minimized.

Copy link

viluon commented Oct 6, 2017

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.

@SquidDev

This comment has been minimized.

Copy link
Contributor Author

SquidDev commented Oct 6, 2017

@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.

@jamierocks

This comment has been minimized.

Copy link

jamierocks commented Oct 6, 2017

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).

@viluon

This comment has been minimized.

Copy link

viluon commented Oct 7, 2017

@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?

@jamierocks

This comment has been minimized.

Copy link

jamierocks commented Oct 7, 2017

@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)

SquidDev added some commits May 1, 2017

Remove string metatable protection
The string metatable and environment are no longer shared, so this
sandboxing is no longer required.
Change timeout system to occur on instructions instead of API calls
This means loops which do not touch CC specific methods will still
produce an error, rather than terminating the computer.
Prefix all loaded strings with "="
Whilst this is not consistent with normal Lua, this is required in order
to remain compatible with LuaJ.

@SquidDev SquidDev force-pushed the SquidDev-CC:feature/cobalt branch from c55d431 to 0bd0f4d Nov 14, 2017

@SquidDev

This comment has been minimized.

Copy link
Contributor Author

SquidDev commented May 1, 2018

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!

Bump Cobalt version
 - Default to using little endian in string.dump
 - Remove propagation of debug hooks to child coroutines
 - Use [C] instead of function name in traceback.
 - Various improvements to string formatting:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.