Skip to content

add ping and pong to message handler #1143

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
BillyONeal merged 17 commits into from
Jun 13, 2019
Merged

add ping and pong to message handler #1143

BillyONeal merged 17 commits into from
Jun 13, 2019

Conversation

@AlexandreK38
Copy link
Contributor

@AlexandreK38 AlexandreK38 commented May 27, 2019 •
edited
Loading

using set_ping_hanlder, set_pong_handler

@msftclas
Copy link

msftclas commented May 27, 2019 •
edited
Loading

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
Dimon4eg
Dimon4eg suggested changes May 28, 2019
View reviewed changes
Release/src/websockets/client/ws_client_wspp.cpp Outdated
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

@Dimon4eg Dimon4eg May 28, 2019

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 suggested changes Jun 6, 2019
View reviewed changes
Copy link
Contributor

@garethsb garethsb left a comment

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.

@garethsb garethsb mentioned this pull request Jun 6, 2019
Closed
@AlexandreK38
Copy link
Contributor Author

AlexandreK38 commented Jun 6, 2019 •
edited
Loading

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

garethsb
garethsb reviewed Jun 6, 2019
View reviewed changes
Copy link
Contributor

@garethsb garethsb left a comment

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.

@garethsb
Copy link
Contributor

garethsb commented Jun 6, 2019 •
edited
Loading

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

@AlexandreK38
Copy link
Contributor Author

AlexandreK38 commented Jun 6, 2019 •
edited
Loading

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

garethsb
garethsb approved these changes Jun 6, 2019
View reviewed changes
Copy link
Contributor

@garethsb garethsb left a comment

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?

BillyONeal
BillyONeal reviewed Jun 7, 2019
View reviewed changes
@BillyONeal BillyONeal merged commit 0f45af1 into microsoft:master Jun 13, 2019
@BillyONeal
Copy link
Member

BillyONeal commented Jun 13, 2019

Thanks for your contribution!

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

Reviewers

@BillyONeal BillyONeal BillyONeal left review comments

+2 more reviewers

@garethsb garethsb garethsb approved these changes

@Dimon4eg Dimon4eg Dimon4eg requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@AlexandreK38 @msftclas @garethsb @BillyONeal @Dimon4eg