Skip to content
This repository was archived by the owner on Jun 1, 2026. It is now read-only.

add ping and pong to message handler#1143

Merged
BillyONeal merged 17 commits into
microsoft:masterfrom
AlexandreK38:ping-pong
Jun 13, 2019
Merged

add ping and pong to message handler#1143
BillyONeal merged 17 commits into
microsoft:masterfrom
AlexandreK38:ping-pong

Conversation

@AlexandreK38

@AlexandreK38 AlexandreK38 commented May 27, 2019

Copy link
Copy Markdown
Contributor

using set_ping_hanlder, set_pong_handler

@msftclas

msftclas commented May 27, 2019

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@AlexandreK38 AlexandreK38 changed the title add ping and pong to message handler, and optional pong timeout add ping and pong to message handler May 27, 2019
incoming_msg.m_msg_type = websocket_message_type::ping;
incoming_msg.m_body = concurrency::streams::container_buffer<std::string>(msg);
// 'move' the payload into a container buffer to avoid any copies.
incoming_msg.m_body = concurrency::streams::container_buffer<std::string>(std::move(msg));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

std::move is redundant here. Compiler will ignore it because msg is const ref. To use std::move the msg should be &&:
std::string&& msg

@garethsb garethsb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good at first review, I'll try to test soon.

Comment thread Release/include/cpprest/ws_msg.h
Comment thread Release/include/cpprest/ws_msg.h Outdated
Comment thread Release/include/cpprest/ws_msg.h
Comment thread Release/include/cpprest/ws_msg.h Outdated
Comment thread Release/include/cpprest/ws_msg.h Outdated
Comment thread Release/src/websockets/client/ws_client_wspp.cpp Outdated
@AlexandreK38

AlexandreK38 commented Jun 6, 2019

Copy link
Copy Markdown
Contributor Author

Btw I added a enable_auto_pong()in the config in order to automatically answer to ping requests

@garethsb garethsb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick updates. A few comments on latest changes.

Comment thread Release/src/websockets/client/ws_client_wspp.cpp Outdated
Comment thread Release/src/websockets/client/ws_client_wspp.cpp Outdated
Comment thread Release/include/cpprest/ws_msg.h
@garethsb

garethsb commented Jun 6, 2019

Copy link
Copy Markdown
Contributor

I've added some unit tests for this to garethsb@468ce71. GitHub won't let me submit a PR on your fork/branch.

@AlexandreK38

AlexandreK38 commented Jun 6, 2019

Copy link
Copy Markdown
Contributor Author

Not sure why it doesn't accept PR but anyway I added your commits :)

@garethsb garethsb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. @BillyONeal would you be able to review this?

Comment thread Release/include/cpprest/ws_msg.h Outdated
@BillyONeal BillyONeal merged commit 0f45af1 into microsoft:master Jun 13, 2019
@BillyONeal

Copy link
Copy Markdown
Member

Thanks for your contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants