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

fix(server/main): prevent multiple vehicle classes requests to client #572

Merged
merged 6 commits into from
Sep 19, 2024

Conversation

D4isDAVID
Copy link
Member

@D4isDAVID D4isDAVID commented Sep 18, 2024

Description

This fixes 3 issues:

  • Calls to this function would always send a new client callback for vehicle classes, since non-array tables don't have a length. Fixed by assigning nil at first and checking for that.
  • lib.callback.await is async. This gives potential for multiple callbacks when vehicle classes aren't available, but are still being waited on by other callers. Fixed by creating a new promise, and resolving it when the callback is done.
  • If the callback failed for one player, other players may still be available. Fixed by asking different players continuously until there's no players to ask.

Might need more of a discussion on the 2nd point. What should happen if the client can't answer for some reason and the callback times out? Should we return nil like in the current solution or throw an error?

Checklist

  • I have personally loaded this code into an updated Qbox project and checked all of its functionality.
  • My pull request fits the contribution guidelines & code conventions.

Copy link
Member

@Manason Manason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Isn't each export invocation in a separate thread though? And yeah we should throw an error rather than return nil.

@D4isDAVID
Copy link
Member Author

D4isDAVID commented Sep 18, 2024

Isn't each export invocation in a separate thread though?

Yes, but why would that matter? The main problem is that the client can be overflowed with events because of the callbacks, in my case eventually kicking me out. Servers with low player counts would suffer, dev servers too where there's only 1 player.

@Manason
Copy link
Member

Manason commented Sep 18, 2024

Isn't each export invocation in a separate thread though?

Yes, but why would that matter? The main problem is that the client can be overflowed with events because of the callbacks, in my case eventually kicking me out. Servers with low player counts would suffer, dev servers too where there's only 1 player.

It should only ever make one callback though, then everything would be cached? How did you overload yourself?

@D4isDAVID D4isDAVID force-pushed the fix/getvehicleclass-overflow branch from 4b2fe51 to 26fdf8b Compare September 18, 2024 03:25
@D4isDAVID
Copy link
Member Author

It should only ever make one callback though, then everything would be cached? How did you overload yourself?

Mainly because the previous code did not correctly check if cache is available, and always assumed it isn't. Another problem spawns where say a first call to this function makes a callback, but while waiting for the client to answer, another call happens. The server is still waiting for the client to answer, but the second call already went through, making another callback. This isn't needed and is solved by creating a promise on the first call and awaiting it in the next calls.

@D4isDAVID D4isDAVID force-pushed the fix/getvehicleclass-overflow branch from 26fdf8b to f636ab2 Compare September 19, 2024 00:58
server/main.lua Outdated Show resolved Hide resolved
@Manason Manason merged commit e1d2b17 into main Sep 19, 2024
1 check passed
@Manason Manason deleted the fix/getvehicleclass-overflow branch September 19, 2024 05:34
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.

3 participants