Skip to content

Handle HTTP errors in DevSupportManager. #4880

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

Merged
JunielKatarn merged 2 commits into from
May 12, 2020
Merged

Handle HTTP errors in DevSupportManager. #4880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
045a53c
Handle HTTP connect exceptions in DevSupportManager
JunielKatarn May 12, 2020
1d57353
Change files
JunielKatarn May 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions change/react-native-windows-2020-05-12-14-18-36-OC-4094136-connectxcpt.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"type": "none",
Copy link
Contributor

@NickGerleman NickGerleman May 13, 2020

Choose a reason for hiding this comment

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

FYI, none means Beachball won't publish a build for this or include it in its changelog. This should be prerelease.

Copy link
Contributor Author

@JunielKatarn JunielKatarn May 13, 2020

Choose a reason for hiding this comment

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

I was on the fence on this one.
When this change is backported to 0.60-stable, I do plan to generate a new version.
for the master branch, didn't think it was worth it.

What criteria would you suggest?
Any change we want reflected on any branch as a new package version should be prerelease?

Copy link
Contributor

@NickGerleman NickGerleman May 13, 2020

Choose a reason for hiding this comment

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

Basically the only time we want to use “none” is when the change doesn’t impact the package (e.g. changing CI). We switched the master branch to a nightly build to reduce version spam, so you shouldn’t need to worry about this making a new build. Having the commit in a change log is nice though, and we would probably want to publish this if it was the only thing committed that day.

"comment": "Handle HTTP connect exceptions in DevSupportManager",
"packageName": "react-native-windows",
"email": "[email protected]",
"dependentChangeType": "none",
"date": "2020-05-12T21:18:36.538Z"
}
14 changes: 12 additions & 2 deletions vnext/Desktop/DevSupportManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,12 @@ string DevSupportManager::GetJavaScriptFromServer(
Url url(bundleUrl);
auto const resolveResult = m_resolver.resolve(url.host, url.port);
tcp::socket socket{m_context};
connect(socket, resolveResult);
boost::system::error_code ec;
connect(socket, resolveResult, ec);
if (ec) {
m_exceptionCaught = true;
return R"({"error:")"s + ec.message() + R"("})"s;
Copy link

@NikoAri NikoAri May 12, 2020

Choose a reason for hiding this comment

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

return [](start = 4, length = 6)

It's strange to return string as error. Why not just have m_errorString? Or some other error pattern?

Copy link
Contributor Author

@JunielKatarn JunielKatarn May 12, 2020

Choose a reason for hiding this comment

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

I disagree. This pattern is similar to an HRESULT or an HTTP code. There is always a return value, but it can be an error type.
Adding an instance member would be unnecessary, if error conditions are indicated by m_exceptionCaught. Only GetJavaScriptFromServer is meant to return a string. If this function fails, we may want a way to indicate the error details.

Copy link
Contributor Author

@JunielKatarn JunielKatarn May 12, 2020

Choose a reason for hiding this comment

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

See OInstance, which consumes the DevSupportManager.

}

request<string_body> request{verb::get, url.Target(), 11};
request.set(field::host, url.host);
Expand Down Expand Up @@ -119,7 +124,12 @@ task<void> DevSupportManager::LaunchDevToolsAsync(const string &debugHost, const
Url url(DevServerHelper::get_LaunchDevToolsCommandUrl(debugHost));
auto const resolveResult = m_resolver.resolve(url.host, url.port);
tcp::socket socket{m_context};
connect(socket, resolveResult);
boost::system::error_code ec;
connect(socket, resolveResult, ec);
if (ec) {
m_exceptionCaught = true;
return;
}

request<string_body> request{verb::get, url.Target(), 11};
request.set(field::host, url.host);
Expand Down