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.

Sign up for GitHub

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

Jump to bottom

Fix syncreport crash caused by getting LocalClient index from ReplayConnection #20532

Merged
merged 1 commit into from Dec 11, 2022
Merged

Fix syncreport crash caused by getting LocalClient index from ReplayConnection #20532

merged 1 commit into from Dec 11, 2022

Conversation

dnqbob
Copy link
Contributor

@dnqbob dnqbob commented Dec 10, 2022

Fix syncreport crash caused by getting LocalClient index from ReplayConnection.

Crash reproduction:

  1. select any mod with replay
  2. change the mcv's HP or anything like that
  3. play replay, get a null reference crash

PunkPun
PunkPun reviewed Dec 10, 2022
View reviewed changes
Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

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

Could we use inline strings here? C# often doesn't handle addition properly when nulls are involved

@dnqbob
Copy link
Contributor Author

dnqbob commented Dec 10, 2022

Ok, fixed

Although in my test the previous version works normal

@PunkPun
Copy link
Member

PunkPun commented Dec 10, 2022

it would be cleaner with the aforementioned inline strings https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/tokens/interpolated

PunkPun
PunkPun reviewed Dec 10, 2022
View reviewed changes
OpenRA.Game/Network/SyncReport.cs Outdated
@@ -104,7 +104,8 @@ void GenerateSyncReport(Report report, IEnumerable<OrderManager.ClientOrder> ord

internal void DumpSyncReport(int frame)
{
var reportName = "syncreport-" + DateTime.UtcNow.ToString("yyyy-MM-ddTHHmmssZ", CultureInfo.InvariantCulture) + "-" + orderManager.LocalClient.Index + ".log";
var reportIndex = orderManager.LocalClient == null ? "" : orderManager.LocalClient.Index.ToString();
var reportName = "syncreport-" + DateTime.UtcNow.ToString("yyyy-MM-ddTHHmmssZ", CultureInfo.InvariantCulture) + "-" + reportIndex + ".log";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var reportName = "syncreport-" + DateTime.UtcNow.ToString("yyyy-MM-ddTHHmmssZ", CultureInfo.InvariantCulture) + "-" + reportIndex + ".log";
var reportName = "syncreport-" + DateTime.UtcNow.ToString("yyyy-MM-ddTHHmmssZ", CultureInfo.InvariantCulture) + $"-{orderManager.LocalClient?.Index}.log";

Copy link
Member

Choose a reason for hiding this comment

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

Why not interpolate the entire string?

Copy link
Member

Choose a reason for hiding this comment

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

can't have strings in strings

Copy link
Member

@PunkPun PunkPun Dec 10, 2022 •
edited

Choose a reason for hiding this comment

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

mb, it appears you actually can. I assumed c# wasn't smart enough.
So then yes, the whole line should be interpolated

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var reportName = "syncreport-" + DateTime.UtcNow.ToString("yyyy-MM-ddTHHmmssZ", CultureInfo.InvariantCulture) + "-" + reportIndex + ".log";
var reportName = $"syncreport-{DateTime.UtcNow.ToString("yyyy-MM-ddTHHmmssZ", CultureInfo.InvariantCulture)}-{orderManager.LocalClient?.Index}.log";

Copy link
Member

Choose a reason for hiding this comment

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

$"syncreport-{DateTime.UtcNow.ToString("yyyy-MM-ddTHHmmssZ", CultureInfo.InvariantCulture)}-{orderManager.LocalClient?.Index}.log" works on dotnetfiddle.

Copy link
Member

Choose a reason for hiding this comment

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

So something like

Suggested change
var reportName = "syncreport-" + DateTime.UtcNow.ToString("yyyy-MM-ddTHHmmssZ", CultureInfo.InvariantCulture) + "-" + reportIndex + ".log";
var timestamp = DateTime.UtcNow.ToString("yyyy-MM-ddTHHmmssZ", CultureInfo.InvariantCulture);
var reportName = $"syncreport-{timestamp}-{orderManager.LocalClient?.Index}.log";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So something like

Changed to that, feels good.

@PunkPun PunkPun merged commit 45d4a2c into OpenRA:bleed Dec 11, 2022
@PunkPun
Copy link
Member

PunkPun commented Dec 11, 2022

Changelog

@dnqbob dnqbob deleted the replay-desync-crash branch December 12, 2022 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PunkPun PunkPun PunkPun left review comments

@abcdefg30 abcdefg30 abcdefg30 approved these changes

Assignees
No one assigned
Labels
PR: Needs +2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dnqbob @PunkPun @abcdefg30