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

IOS/NET: Add timeout on blocking connect #8759

Merged
merged 1 commit into from Feb 21, 2021

Conversation

sepalani
Copy link
Contributor

This PR adds a timeout on connect for blocking sockets based on this hardware test. Beware that it tries to connect to an ubisoft server (which timeout after ~3 minutes on hardware).

wii-connect-timeout.zip

I'll need to reverse IOS to confirm that behaviour, however. The timeout value (in second) can also be changed in the config file under the [Network] section.

Source/Core/Core/IOS/Network/Socket.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/Socket.h Outdated Show resolved Hide resolved
@sepalani
Copy link
Contributor Author

@leoetlino
Done. I'm still open for a better name regarding the DoTimeout() method.

@leoetlino
Copy link
Member

StartOrCheckTimeout maybe? Naming is hard...

@iwubcode
Copy link
Contributor

iwubcode commented May 6, 2020

I'm not great at naming either. I can only offer my thoughts. For boolean functions, I usually use 'is' or 'has', so maybe HasTimedout() ?

// Main.Network

const ConfigInfo<int> MAIN_NETWORK_TIMEOUT{{System::Main, "Network", "NetworkTimeout"},
190}; // Based on https://dolp.in/pr8759 hwtest
Copy link
Member

Choose a reason for hiding this comment

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

You may wish to relocate this comment on top of the definition so that the wonky formatting can be avoided.

The comment can just be formatted to indicate it means the default value.

// Default value based on https://dolp.in/pr8759 hwtest

@sepalani
Copy link
Contributor Author

sepalani commented May 7, 2020

@lioncash
Done.

@iwubcode
As @leoetlino said, the initial name was HasTimeout but the function doesn't only check for a boolean value but starts the timer on-the-fly if needed and then checks if a timeout occurred.

@sepalani
Copy link
Contributor Author

Rebased on latest master, I also added a reference to an RFC regarding the TCP connection failures.

Source/Core/Core/Config/MainSettings.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/Socket.cpp Outdated Show resolved Hide resolved
@sepalani
Copy link
Contributor Author

@leoetlino
Done. Rather than splitting it into 2 functions, I decided to create simple GetTimeout function and perform the check inside the if statement.

@@ -184,6 +186,7 @@ class WiiSocket
WiiSocket& operator=(WiiSocket&&) = default;

private:
using Timeout = std::chrono::time_point<std::chrono::system_clock>;
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I missed something important in my previous review: this should be using a steady_clock, not a system_clock. The latter isn't monotonic and can be changed at any time by the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I completely forgot about that.

I also updated the RFC comment since it's recommending this timeout to be at least 3 minutes.

@leoetlino leoetlino merged commit c040b01 into dolphin-emu:master Feb 21, 2021
9 checks passed
@sepalani sepalani deleted the so-connect branch February 21, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants