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

Implement a new console input system to fix an issue with the tmux send-keys command #255

Merged
merged 8 commits into from May 24, 2021

Conversation

iRebbok
Copy link
Contributor

@iRebbok iRebbok commented Feb 10, 2021

When you send some message to a tmux session, which runs MA, it reads the first key of the message and then falls in a loop with Console.KeyAvailable.
Somehow Console.KeyAvailable returns false after the first key is read, this part of code causes the issue

while (!Console.KeyAvailable)
{
await Task.Delay(10, cancellationToken);
}

My fix is to just remove all calls to WaitForKey(CancellationToken), however, Console.ReadKey() always works and it blocks the task of reading input, are those CancellationTokenSources needed at all?

Related to #252.

This PR implements a new console input system and removes the use_new_input_system config entry.
The default value of the new console_input_system config entry is New (as before).

@ButterscotchV
Copy link
Collaborator

This prevents MultiAdmin from shutting down properly

@ButterscotchV
Copy link
Collaborator

This is not related to issue #252, that issue is to do with mono trying to print to a non-existent console window, therefore causing an error, while this pull request appears to be related to something entirely different.

The reason that MultiAdmin has been changed to use Console.KeyAvailable is because Console.ReadKey() is forcibly blocking no matter what, so there's no possible way to cancel it once the server is shutting down, this is what the cancellation token is used for.

@iRebbok
Copy link
Contributor Author

iRebbok commented Feb 10, 2021

Hmm, when I debugged in mono, it returned true for Console.KeyAvailable only once (i.e. only the first letter) if I sent it using tmux send-keys. The issue I mentioned seems to contain two issues: the first is tmux send-keys not working (related to input) and the second is printing to a non-existent console cause an exception (related to output).

Also I was debugging with default settings (enabled the new input system and disabled hiding console input) and it didn't throw an exception regarding Console.ReadKey even it was a non-existent console.

As there's an issue with Console.KeyAvailable, I think fixing has a higher priority than properly closing the read input task. Also MultiAdmin doesn't support server console switching yet, and when it will, there shouldn't be different read input tasks for different servers, so technically force canceling the read input task doesn't affect anything.

@ButterscotchV
Copy link
Collaborator

ButterscotchV commented Feb 10, 2021

As there's an issue with Console.KeyAvailable, I think fixing has a higher priority than properly closing the read input task. Also MultiAdmin doesn't support server console switching yet, and when it will, there shouldn't be different read input tasks for different servers, so technically force canceling the read input task doesn't affect anything.

It's required to be cancelled for shutting down MultiAdmin.... It doesn't matter if there's console switching or what, but currently it's a big issue.

I think instead, this should be fixed in Mono, instead of trying to make some janky workaround in MultiAdmin that breaks functionality in another place.

@iRebbok
Copy link
Contributor Author

iRebbok commented Feb 10, 2021

I think instead, this should be fixed in Mono, instead of trying to make some janky workaround in MultiAdmin that breaks functionality in another place.

I'd test it on .NET 5, since I haven't tested it on .NET 5 yet.

@ButterscotchV
Copy link
Collaborator

I think instead, this should be fixed in Mono, instead of trying to make some janky workaround in MultiAdmin that breaks functionality in another place.

I'd test it on .NET 5, since I haven't tested it on .NET 5 yet.

Sounds like a good idea, it wouldn't be worthwhile to try to fix something that's already fixed elsewhere. I want to get MultiAdmin on .NET 5 released soon, but it'd require a huge change in how the releases are structured and I want there to be some sort of semi-stable release out for the new version first.

Maybe a new config option to just use Console.ReadLine() like before would be in order? It'd need to have a big warning for possibly preventing MultiAdmin from closing, but it would work around most of the bugs that have arisen with the latest version and could be pushed as a stable release.

@iRebbok
Copy link
Contributor Author

iRebbok commented Feb 10, 2021

Maybe a new config option to just use Console.ReadLine() like before would be in order? It'd need to have a big warning for possibly preventing MultiAdmin from closing, but it would work around most of the bugs that have arisen with the latest version and could be pushed as a stable release.

I think it's great, I'm going to do that after I get a result of testing the issue on .NET 5.

@ButterscotchV
Copy link
Collaborator

Maybe a new config option to just use Console.ReadLine() like before would be in order? It'd need to have a big warning for possibly preventing MultiAdmin from closing, but it would work around most of the bugs that have arisen with the latest version and could be pushed as a stable release.

I think it's great, I'm going to do that after I get a result of testing the issue on .NET 5.

I'm not sure whether it would be too confusing, but it could be a lot simpler for it to just be an integer value for selecting the input system, like using 0 for Console.ReadLine(), 1 for the "old" input system, and 2 for the "new" input system, then any additional input systems could be added later.

@iRebbok
Copy link
Contributor Author

iRebbok commented Feb 10, 2021

I've got a great result. It works perfectly on .NET 5 except it freezes on close. Probably it's the same thing that happened with LocalAdmin, it was falling in a loop on close.

@iRebbok
Copy link
Contributor Author

iRebbok commented Feb 10, 2021

I think it's now ready to be reviewed.

@iRebbok iRebbok changed the title fix key not being detected if tmux send-keys is used Implement a new console input system to fix an issue with the tmux send-keys command Feb 10, 2021
README.md Outdated Show resolved Hide resolved
@ButterscotchV ButterscotchV merged commit 8961c7c into ServerMod:master May 24, 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.

None yet

2 participants