Skip to content

Remove pplx::task from public API #8747

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
Merged
merged 9 commits into from
Mar 30, 2019
Merged

Remove pplx::task from public API #8747

merged 9 commits into from
Mar 30, 2019

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Mar 22, 2019 •
edited
Loading

Built on top of #8420

Part of #8718

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Mar 22, 2019
@BrennanConroy BrennanConroy changed the base branch from brecon/lowapi to master March 25, 2019 22:06
@analogrelay analogrelay added this to the 3.0.0-preview4 milestone Mar 25, 2019
mikaelm12
mikaelm12 reviewed Mar 28, 2019
View reviewed changes
src/SignalR/clients/cpp/src/signalrclient/connection.cpp
{
return m_pImpl->start();
m_pImpl->start(callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need checks for the implementation object here as well?

src/SignalR/clients/cpp/samples/HubConnectionSample/HubConnectionSample.cpp
}

ucout << U("Enter your message:");
for (;;)
Copy link
Contributor

Choose a reason for hiding this comment

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

while true lol

Copy link
Member

Choose a reason for hiding this comment

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

👍 for for (;;) lol

@BrennanConroy BrennanConroy marked this pull request as ready for review March 29, 2019 15:55
halter73
halter73 reviewed Mar 29, 2019
View reviewed changes
src/SignalR/clients/cpp/include/signalrclient/connection.h

SIGNALRCLIENT_API pplx::task<void> __cdecl send(const std::string& data);
SIGNALRCLIENT_API void __cdecl send(const std::string& data, std::function<void(std::exception_ptr)> callback) noexcept;

SIGNALRCLIENT_API void __cdecl set_message_received(const message_received_handler& message_received_callback);
SIGNALRCLIENT_API void __cdecl set_disconnected(const std::function<void __cdecl()>& disconnected_callback);
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to have set_disconnected's completion callback take an exception as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, I'll track that in our public API review issue #8717

halter73
halter73 reviewed Mar 29, 2019
View reviewed changes
src/SignalR/clients/cpp/include/signalrclient/connection.h
@@ -29,16 +28,16 @@ namespace signalr

connection& operator=(const connection&) = delete;

SIGNALRCLIENT_API pplx::task<void> __cdecl start();
SIGNALRCLIENT_API void __cdecl start(std::function<void(std::exception_ptr)> callback) noexcept;
Copy link
Member

Choose a reason for hiding this comment

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

Can these callbacks be marked noexcept too? std::function<void(std::exception_ptr) noexcept>

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it, but I don't think it works
https://godbolt.org/z/dI2GuJ

halter73
halter73 reviewed Mar 29, 2019
View reviewed changes
src/SignalR/clients/cpp/src/signalrclient/hub_connection.cpp
{
if (!m_pImpl)
{
throw signalr_exception("invoke() cannot be called on uninitialized hub_connection instance");
callback(web::json::value(), std::make_exception_ptr(signalr_exception("invoke() cannot be called on destructed hub_connection instance")));
Copy link
Member

Choose a reason for hiding this comment

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

Is web::json::value() equivalent to null in this case?

halter73
halter73 reviewed Mar 29, 2019
View reviewed changes
src/SignalR/clients/cpp/src/signalrclient/hub_connection_impl.cpp
auto handshake_exception = std::current_exception();
connection->m_connection->stop([callback, handshake_exception](std::exception_ptr)
{
callback(handshake_exception);
Copy link
Member

Choose a reason for hiding this comment

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

Can we merge or log the stop exception if there is one?

Copy link
Member Author

Choose a reason for hiding this comment

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

We log the exception in connection_impl::stop

halter73
halter73 reviewed Mar 29, 2019
View reviewed changes
src/SignalR/clients/cpp/src/signalrclient/hub_connection_impl.cpp Outdated
set_result(json::value::null());
else
{
set_result(json::value::null());
Copy link
Member

Choose a reason for hiding this comment

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

Should this be web::json::value() instead? The docs seem to indicate that value() is just a null value and null() is "A JSON null value".

@BrennanConroy
Copy link
Member Author

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=138178&view=logs

@aspnet-hello
Copy link

This comment was made automatically. If there is a problem contact ryanbrandenburg.

I've triaged the above build.

@BrennanConroy BrennanConroy merged commit 9d8990a into master Mar 30, 2019
@BrennanConroy BrennanConroy deleted the brecon/publicapi branch March 30, 2019 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikaelm12 mikaelm12 mikaelm12 left review comments

@halter73 halter73 halter73 approved these changes

@davidfowl davidfowl Awaiting requested review from davidfowl

@analogrelay analogrelay Awaiting requested review from analogrelay

@jkotalik jkotalik Awaiting requested review from jkotalik

Assignees
No one assigned
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Milestone
3.0.0-preview4
Development

Successfully merging this pull request may close these issues.
5 participants
@BrennanConroy @aspnet-hello @halter73 @mikaelm12 @analogrelay