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

C++ API WebSocket example #624

Closed
wants to merge 7 commits into from
Closed

Conversation

ogoodman
Copy link
Contributor

There are two commits. The first one adds an API function to determine whether a transaction is a websocket request and adds code to the InterceptPlugin to make it behave appropriately. The second one provides a working demo of a websocket handler.

pos_ += 2;
for (int i = 0; i < 2; ++i, ++pos_) {
msg_len <<= 8;
msg_len += (unsigned char)(ws_buf_[pos_]);
Copy link
Member

@bgaff bgaff May 10, 2016

Choose a reason for hiding this comment

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

You're neglecting network byte order to host order conversions, you need to use nthos:

ntohs(*(uint16_t *) (ws_buf_ + pos_));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code did handle network byte order correctly, but it is true that ntohs and friends are more efficient on big-endian machines.

Adds constants and enums for readability. Adds proper handling
of most opcodes including pings. Removes timeout settings.

using namespace atscppapi;

#define SAY(a) std::cout << a << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

Please see the stuff we have available in atscppapi/Logger.h, such as TSDebug or creating a logger class.

@bgaff
Copy link
Member

bgaff commented May 11, 2016

With the C plugins we've had an experimental directory for plugins that were fairly new, we should probably consider adding this to a folder under atscppapi/examples/experimental for now and then we can always promote it later. Thanks for contributing.

upgrade headers, framing and decoding, and a much simplified plugin.
@zwoop zwoop added the Plugins label May 12, 2016
@zwoop zwoop added this to the 7.0.0 milestone May 15, 2016
@bgaff
Copy link
Member

bgaff commented May 31, 2016

Hi @ogoodman , I want to get this landed soon can you please just make one final change which puts it under an experimental/ folder in examples? After that I'm happy to commit this for you.

Thanks

@shukitchan
Copy link
Contributor

also is there a jira ticket for this?

@jpeach
Copy link
Contributor

jpeach commented May 31, 2016

@bgaff The new API should go through API review. IMHO it would be best to make a separate JIRA and pull request for the API changes.

@bgaff
Copy link
Member

bgaff commented May 31, 2016

@ogoodman would you mind also starting a thread on dev@trafficserver.apache.org for API review? I can also kick off this discussion if you'd like.

@zwoop
Copy link
Contributor

zwoop commented Jun 27, 2016

This needs a rebase as well before we can proceed.

@zwoop
Copy link
Contributor

zwoop commented Jun 27, 2016

Also, repeating the question above, what is the Jira for this?

@ogoodman
Copy link
Contributor Author

There is no Jira. I just thought it might be of interest to people as another example plugin. If there is interest I can open one describing this as an enhancement. Shall I do that?

@zwoop
Copy link
Contributor

zwoop commented Jun 28, 2016

Yeah, examples are great. So please create a Jira, if you are going to work on it. Any changes to APIs would need to go through API review though, so depending on what you are doing, you might want to split this up into multiple commits and/or multiple Jira's and PRs.

@zwoop
Copy link
Contributor

zwoop commented Jul 23, 2016

Status on this PR? Is there a Jira? The branch has conflicts now, and likely might need a clang-format run as well.

@ogoodman
Copy link
Contributor Author

I'm going to close this particular PR now as it has become messy, doesn't reference any JIRA issue, and needs to be split into a PR on the TS API followed by one which adds the example to the C++ API.
JIRA issues TS-4698 and TS-4699 have been created for the two improvements.

@ogoodman ogoodman closed this Jul 23, 2016
@zwoop zwoop modified the milestone: 7.0.0 May 4, 2017
@zwoop zwoop unassigned bgaff May 4, 2017
shinrich added a commit to shinrich/trafficserver that referenced this pull request Jan 9, 2018
Add comment regarding missing header file to gzip.cc.
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Sep 26, 2023
Co-Authored-By: Bryan Call <bcall@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants