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

Execute ConVarQueryFinished if client disconnects #1384

Merged
merged 5 commits into from
Mar 15, 2021
Merged

Execute ConVarQueryFinished if client disconnects #1384

merged 5 commits into from
Mar 15, 2021

Conversation

Deathreus
Copy link
Contributor

So people passing handles to the extra data can close them, adds a new return value to check if this happened

So people passing handles to the extra data can close them, adds a new return value to check if this happened
Copy link
Member

@asherkin asherkin left a comment

Choose a reason for hiding this comment

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

This all looks sane to me. I'll leave it open for a bit in case any one else has any comments on the use of -1 (I saw a bit on Discord this morning.) I think the only real risk here is SMAC's convar checks, so as long as that's not going to start banning people when getting the value here I'm happy!

core/ConVarManager.cpp Outdated Show resolved Hide resolved
@psychonic
Copy link
Member

It might be worth exposing a feature capability so that plugins that want or need to require this can know if it would ever be called.

Co-authored-by: Asher Baker <asherkin@limetech.io>
Copy link
Member

@KyleSanderson KyleSanderson left a comment

Choose a reason for hiding this comment

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

This doesn't feel quite right - see comments please.

core/ConVarManager.cpp Show resolved Hide resolved
core/ConVarManager.cpp Outdated Show resolved Hide resolved
core/ConVarManager.cpp Outdated Show resolved Hide resolved
@peace-maker
Copy link
Member

What happened here?

@Deathreus
Copy link
Contributor Author

What happened here?

Oopsie, pulled into here on accident, let me see if I can clean it up

Copy link
Member

@KyleSanderson KyleSanderson left a comment

Choose a reason for hiding this comment

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

I agree we should handle this case, but the only avenue the client (readas: script caller) has to reconcile this is to stay on the QueryCookie. When we return 0 that still leaves the initial issue present when there's multiple inflight cases. It's like you're in prison, have two people with the name Wyatt, and get two callbacks for Wyatt but don't know who's clothes are who. You can certainly guess, but you're going to be wrong half the time (and more with the more callbacks outstanding).

{
cell_t ret;

pCallback->PushCell(0);
Copy link
Member

Choose a reason for hiding this comment

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

Where's this value?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch, it looks like it should be available as query.cookie.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point, but I'm not sure how the cookie is needed? I added the value but a more clear explanation rather than an analogy would be appreciated, thanks

@KyleSanderson KyleSanderson merged commit 46c54f8 into alliedmodders:master Mar 15, 2021
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.

5 participants