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

[0.63] Allow propagating WinRTWebSocketResource constructor exceptions #7898

Merged
5 commits merged into from
Jun 2, 2021

Conversation

JunielKatarn
Copy link
Contributor

@JunielKatarn JunielKatarn commented May 30, 2021

Backport of #7892.

Microsoft Reviewers: Open in CodeFlow

JunielKatarn and others added 2 commits May 29, 2021 18:03
…osoft#7892)

* Correctly format message on hresult_error

* Remove noexcept from public constructors
@JunielKatarn JunielKatarn requested review from a team May 30, 2021 01:07
@JunielKatarn JunielKatarn added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Jun 1, 2021
@ghost
Copy link

ghost commented Jun 1, 2021

Hello @JunielKatarn!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

WinRTWebSocketResource::WinRTWebSocketResource(
IMessageWebSocket &&socket,
IDataWriter &&writer,
Uri &&uri,
vector<ChainValidationResult> &&certExeptions) noexcept
Copy link

Choose a reason for hiding this comment

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

noexcept

(nit) why? which exceptions are expected to be thrown from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2 reported at least:

  1. Malformed Uri (i.e. Uri{""})
  2. Plain websocket instantiation failure (unknown cause): MessageWebSocket{}

Copy link

Choose a reason for hiding this comment

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

Maybe worth adding a comment that such exception types could be thrown, so that consumers are at least aware?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wish the WinRT docs did that in the first place 😅

This should not be necessary because after this fix the exception will propagate to the JS layer and can be handled gracefully.

While 2.'s causes are still unclear, 1. is actually very expected if a malformed URI is passed to a component meant to establish a connection.

Copy link

@NikoAri NikoAri left a comment

Choose a reason for hiding this comment

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

:shipit:

@JunielKatarn JunielKatarn removed the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Jun 2, 2021
@JunielKatarn
Copy link
Contributor Author

Awaiting unrelated fix #7918.

@JunielKatarn JunielKatarn added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Jun 2, 2021
@ghost ghost merged commit 4026d6b into microsoft:0.63-stable Jun 2, 2021
@JunielKatarn JunielKatarn deleted the ws-handlebaduri-63 branch June 2, 2021 21:45
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants