Citizen.Await


#1

Citizen.Await seems to return nil sometimes instead of a table at this line https://github.com/citizenfx/fivem/blob/master/data/shared/citizen/scripting/lua/scheduler.lua#L577


#2

any repro other than ‘sometimes’?


#3

Hard to debug, stack traces are not clear, so no, it’s related to coroutines.


#4

Still an issue, I can’t help because the scheduler code is satanic to me, but the people who made it could investigate this.


#5

good job providing no additional info!

ask @Brouznouf, though from what I hear he’s not doing anything FiveM-related lately, so you’ll really have to provide more information as we didn’t make it.


#6

Seriously ? I’m well aware that solving issue is difficult and I value giving most information as possible, but there is a point where it is irrelevant, this is a design flaw, not something a user of the API can probably solve. I could rewrite part of the scheduler, but that would require your approval and some guidelines and will also led to a very different code.


#7

what is a design flaw? the function is meant to only return table in this scenario, and you provided 0 information that can repro the scenario you encounter where it doesn’t.

None of this needs a ‘rewrite’ to resolve at all, and since you apparently know better and don’t provide any information to fix things why don’t you write ‘a very different code’ and PR it, so it can be reviewed?


#8

Just checked code flow around there, there’s no way whatsoever that it can return nil - promise rejection will lead to a Lua error being caused in the coroutine, not a nil return value from Citizen.Await, and a truthy value will return said value.

Did you test this on the latest game version (server pipeline 822 or higher, client updater version 1126140+)?


#9

The usage of coroutines looks wrong, leading to a lot of edge cases, but I will know more about it if I try a rewrite.

First, I don’t know what is your PR policy, so I don’t want to work for nothing, secondly I don’t know what are exactly the requirements expected from the scheduler, changes could break its usage.

There is an issue, you may not care, but I do, and it has been 8 months, so I don’t understand how pointing this out should give me those kind of messages ?

I will look into it (and will probably need answers to do so).


#10

I can focus on just rewriting the threads managment.

Is the “current thread” variables useful for something (beyond internal managment) ?

Do promises are actually required or used by people beyond yielding the coroutine ?


#11

there’s really no PR policy afaik, whatever pleases the elements will get PR’d, but an element may know more. Also nice job again evading the question and not specifying what these “edge cases” are :clap: .


#12

You can see the idea here https://github.com/ImagicTheCat/fivem/commit/095cbb2023dbf7010bd0ca3bd94ce1d8b3a95122.

It’s without optimization.
I don’t know how to handle the promise stuff (because I don’t use it and don’t understand the concept), and can’t really test it because my resources depend on Await.


#13

Concept makes sense but doesn’t really seem necessary to fix any bugs - yeah it makes internal bookkeeping easier but this bug doesn’t seem triggered by internal bookkeeping at all. :confused:

Also yes, promises are an exposed API and definitely need to be implemented correctly, there’s actually people using this.


#14

Incredible… That’s actually the problem, they seem to not be implemented/used properly because of the previous complexity, there is probably a premature promise solved check that causes returning a nil value. I don’t use promises, but the part in comments probably needs to handle two cases:

  • if already solved before yield, return value
  • if not already solved, yield and handle promise event for resume

Anyway, there is a lot of global variables in the original version, it’s expected to have random issues.


#15

Yes, but the same for rejecting and throwing an error. error needs to be thrown within the target coroutine, as well, to allow pcall to catch it.


#16

That was not the point of my changes, just a secondary effect. This way is more straightforward, you only resume what you yield, so other stuff using coroutines which are following this rule can all work together.


#17

Hey,

I’m actually using the promise / await system without any problem (and with vrp, but an old version), so it’s hard to understand what’s wrong here. However i do not have early returning promise so it may be because of that.

For me the only flaw was having this lib was to returning a nil value on error, but it seems to have been fixed by last commits.

It may come from the promise implementation, the current one is from https://github.com/zserge/lua-promises and there seems to be issues with some errors not being rejected correctly

The promise stuff is necessary in order to have a consistent API for the Await API (no random callback definition). Another API may be used but this is a very common standard that a lot of people know so i don’t see the point of reinventing the wheel here.


#18

I carefully recopied the Citizen.Await function to keep it’s actual behavior, while reworking the coroutines.

It seems to solve the issue (which wasn’t expected), but even if it’s not, now it’s possible to properly use the coroutines without relying on Citizen.Await and promises. Everyone wins. No idea about performance, but I don’t have anything significant to benchmark.

The PR: https://github.com/citizenfx/fivem/pull/157

I still think promise are a bad thing coming from a world without coroutines, especially in Lua. But here is the compromise.


#19

I agree that callback / promise are not needed, however there is a ton of library using callback system, having promise + await thing allow a smooth transition

Also very few people actually understand how coroutines works so it allows them to use callback while still allowing other people like you and me to not use them, after all it’s only a question on how to write of code, result is the same.


#20

But that was not the case before my fix, I was forced to use Citizen.Await and promises because of the way coroutines are managed.

Also, the issue is not punctual/random anymore, Citizen.Await will constantly fail without this fix with vRP 2 (my project, dev branch) when the DB driver is registered, so you actually can reproduce it.