Skip to content

Avoid losing console messages. #1071

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

Merged
merged 20 commits into from
Feb 28, 2020

Conversation

BotoX
Copy link
Contributor

@BotoX BotoX commented Sep 2, 2019

Buffers up to 16k bytes of SVC_Print if buffer would overflow, then sends chunks every frame.
Sends up to 2048 bytes per frame and does not split messages.

Only tested on cstrike, works without any changes on plugins.
On client the order is still messed up sometimes because the client itself fucks up the order (also on console commands like find)

Copy link
Member

@peace-maker peace-maker left a comment

Choose a reason for hiding this comment

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

This is great! This'll help getting some console logs in deterministically in order.
Some notes inline.

size_t nMsgLen = strlen(szMsg);
int nNumBitsWritten = pNetChan->GetNumBitsWritten(false);

// 2048 = SVC_Print buffersize (-1 for \0)
Copy link
Member

Choose a reason for hiding this comment

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

Some named constants instead of these magic values would be nice. Are they the same for all engines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bool SVC_Print::WriteToBuffer( bf_write &buffer )
{
	buffer.WriteUBitLong( GetType(), NETMSG_TYPE_BITS );
	return buffer.WriteString( m_szText ? m_szText : " svc_print NULL" );
}

NETMSG_TYPE_BITS is 5 in 2007 code, can't tell for sure if it's 6 in sdk2013 or I did that by trial&error or for safety reasons.
2048 is the size of the buffer in SVC_Print (also 2007 code), -1 for terminating \0 at the end of the string.
This just checks if the string is too big to fit into the current SVC_Print buffer so it doesn't have to be 100% accurate, the 2048 bufsize in SVC_Print is the most important.

Copy link
Member

@Headline Headline left a comment

Choose a reason for hiding this comment

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

Just some cleanup, no need for us to have our own list and manage it

@peace-maker
Copy link
Member

We should reset the buffer in CPlayer::Disconnect.

@Headline
Copy link
Member

Headline commented Sep 3, 2019

I'm cool with code quality, here. Dismissing the rest to others

@KyleSanderson KyleSanderson requested a review from Fyren September 5, 2019 19:59
@Fyren
Copy link
Contributor

Fyren commented Sep 6, 2019

Has this been tried on something other than BotoX's CSS servers?

@BotoX
Copy link
Contributor Author

BotoX commented Sep 6, 2019

I didn't try it anywhere else, but SVC_Print should be the same across all games/engines.
I mean really all this does is buffer messages when the unreliable netchan has more than ~2000 bytes in it and send them in the next tick.
There's barely anything else that uses the unreliable netchan, especially not anything as big as SVC_Print.
So it could be less performant in other games if other stuff uses up the unreliable netchan and then console messages would be a bit slower.
However that's still better than outright losing them.

It obviously makes sense to test this on other games before merging, just put 64 bots on a server and type status. (should yield more than 2048 characters)
Without this patch the message will be cut off and you'll get this error:
Netchannel: failed reading message svc_Print from <serverip>

@KyleSanderson
Copy link
Member

I've likely bikeshedded this to death. Just needs testing then should be good to ship.

@BotoX BotoX force-pushed the feature-SVC_Print-Buffer branch from 55fef9f to f3a5c83 Compare September 10, 2019 09:25
@nosoop
Copy link
Contributor

nosoop commented Feb 1, 2020

Verified working on CS:GO and TF2 (Linux) with the PR merged into branch 1.10-dev. Tested with the following plugin:

#pragma semicolon 1
#include <sourcemod>

#pragma newdecls required

public void OnPluginStart() {
	RegAdminCmd("sm_overflowsvcprint", OverflowConsoleBuffer, ADMFLAG_ROOT);
}

public Action OverflowConsoleBuffer(int client, int argc) {
	for (int i; i < 512; i++) {
		PrintToConsole(client, "%04d: %08x %08x %08x %08x %08x %08x %08x %08x",
				i, GetURandomInt(), GetURandomInt(), GetURandomInt(), GetURandomInt(),
				GetURandomInt(), GetURandomInt(), GetURandomInt(), GetURandomInt());
	}
	return Plugin_Handled;
}

@KyleSanderson KyleSanderson merged commit cc6059a into alliedmodders:master Feb 28, 2020
@KyleSanderson
Copy link
Member

Thank you very much, sorry for the huge delay.

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.

6 participants