Don't deadlock with OpenComputers

[habnabit]

The previous implementation of waitForFuture under OpenComputers was to sleep until the future had a result, but that can cause a deadlock.

The specific deadlock I ran into was that the server thread was saving the world, which requires a lock on the OpenComputers machine. At the same time, the OpenComputers worker thread had acquired the same machine's lock, and was sleeping while waiting for the server to set a result on the future.

Since raising LimitReachedException is effectively just saying 'try again later', there's no harm in doing it this way.

I know you were raising this exception before and then stopped, but it turns out you kind of have to. Sorry!

[bdew]

This solution just breaks it in a different way. Throwing LimitReachedException makes OC redo the call in server thread - where we can't sleep. If the future is not done the only thing it can do is throw an error that will be returned to lua code.

Everything just fails randomly all the time making the interface pretty much useless.

I honestly don't see a good way out of this. I'm tempted to either:

• Drop the idea of supporting OC and CC with the same code
• Drop the idea of doing any fancy concurrency stuff (which would seriously limit some future applications i wanted to do with this stuff)
• Drop OC support entirely

[habnabit]

Throwing LimitReachedException makes OC redo the call in server thread

I don't think this is true. My understanding from reading the code and asking about this failure on the #oc IRC is that throwing LimitReachedException is how you indicate you're yielding, and it'll be retried later in a worker thread again.

[bdew]

The documentation here says:

... The limit itself is enforced in the machine's invoke method, which will throw a LimitReachedException once there were too many call. At that point the architecture should either perform a short sleep, or fall back to perform a synchronized call instead.

(in this context synchronized = in server thread, direct = in worker thread)

So i guess it can go both ways? I couldn't find the relevant code in OC, been a while since i looked at it.

At any rate, even if it sleeps it's still not good. An exception is not a yield, and the machine will just repeat the same call so i have no easy way to link it to the previously generated Future. So a new Future will be started, which won't finish because waitForFuture is called literally in the next line after the future is started - and it will just keep failing forever.

[habnabit]

An exception is not a yield, and the machine will just repeat the same call so i have no easy way to link it to the previously generated Future.

Ah, yeah, I think this is the fundamental problem. I don't know how I didn't see that!

For reference, almost all of my async programming experience is with a single-threaded event loop model, so doing things like spawning threads and blocking on futures is, for now, still outside my reasoning ability.

What kind of fancy concurrency stuff did you even want to do, though? Skimming through the methods currently exposed on data ports, I'm just not sure what you'd have in mind. It seems like it might be more straightforward to remove all of the bits about futures entirely. But, from looking over the wrapInFuture definition, aren't you running all your code in the server thread anyway? Maybe you just want to force all calls to be synchronized? In that case, the fix would be even simpler: raise LimitReachedException any time you see a direct call.

[hron84]

Just an idea: why not ask from @fnuecke about how OpenComputers works in this term?

[fnuecke]

@bdew is correct, LimitReachedException will cause the method to be called again from the server thread, so that's not really an option when waiting via sleeps.

Without knowing the exact use-cases for these futures, might I suggest relaying the waiting to the Lua side of things instead? That would avoid locking up a worker thread in OC / potential too long without yielding errors in CC.

A relatively clean way would be to pass a future-like object to Lua (in OC userdata is implemented using the Value interface (and AbstractValue base class for convenience), in CC there's ILuaObject).

On the Lua side you could then have something like

local future = blah.yourCallback()
while not future.isCompleted() do
  [os.]sleep(10)
end
-- use future.get() or whatever

[habnabit]

There, I did it.

It's backward-incompatible, but that's an implementation to discuss/iterate on, at least. Is it possible to make this backward-compatible while also not totally breaking on OpenComputers?

[bdew]

Just for clarification - am i missing something or PersistableFutureState &co doesn't actually persist the result in any way, even if the future is completed?

I have a preliminary solution that I'm testing and fixing some small leftover bugs and will push it to the public repo in a few hours. I'm sorry that i stalled on this for so long.

Thanks to everyone that contributed, especially @habnabit - while i didn't end up using any of your code, it gave me alot of inspiration to fixing this mess :P

[habnabit]

@bdew I wasn't sure about the best way to do it, and finished the implementation of that after pushing everything else. I'll push the rest now.

[bdew]

So hopefully i've fixed all problems with OC without breaking anything in CC.

Beta build available here: http://minecraft.curseforge.com/projects/pressure-pipes/files/2276873

I've closed all related issues, if there are any unsolved or new problems or other concerns - comment here or open a new issue.

Thanks again to all who contributed.

---

The gory details for those interested:

• Code was split into mod-agnostic base part and separate parts for OC and CC
  • If no issues pop up, some of that will be moved to BDLib and used in my other mods
• This should be modular enough to add support for other computer mods in the future
• Methods can be declared as normal (can only be called in server thread) or direct (can be called anywhere)
  • Calls to direct methods are executed immediately
  • CC calls to non-direct methods are executed in server thread using futures
  • OC calls to non-direct methods throw LimitReachedException which causes OC to re-call in server thread
  • For Tank Dataports everything that reads state is direct, everything that changes it is non-direct. I'm not sure that this is right because there's like 0 mentions of thread safety in forge/minecraft. If this ends up breaking stuff i'll make everything non-direct.
• Methods can return Futures (not used in current code, but will be in the... future)
  • Futures don't have to be the top-level result, it can be nested in a list, map, etc.
  • Calls from CC wait for all futures in the result to finish before returning (using the old method)
  • Calls from OC return the futures wrapped in a lua object (as suggested by @fnuecke)
  • Future results/errors can are serialized if they are complete, otherwise an error is returned after reloading (based on implementation by @habnabit)

[habnabit]

Nice! This seems like a much more sensible solution. Still learning a lot about minecraft/scala dev.

[hron84]

@bdew if the beta commits land in release, will you enable Tank Data Ports if only OC installed?

[bdew]

They already should work with only OC in the beta (if they don't - please report), and will work when it becomes release as well.

[hron84]

@bdew cool, I'll test it out as soon as possible