Add classes for convenient manipulation in plugins of components of HTTP messages. #5812

ywkaras · 2019-08-09T22:25:44Z

These classes may be used independently of other parts of the TS CPP API.

SolidWallOfCode · 2019-08-12T18:10:20Z

This code might be interesting to look at as a comparison.

include/tscpp/api/HttpMsgComp.h

SolidWallOfCode · 2019-08-12T18:12:50Z

include/tscpp/api/HttpMsgComp.h

+  ~MimeField();
+
+private:
+  MsgBase &_msg;


I think it's better to copy over the MBuffer and MLoc instead of carrying a reference. It's cheap to do and removes any scope dependencies between them. It's quite possible to envision logic that gets the field and passes it on after the base object has gone out of scope.

I can't envision logic where it would be desirable to have multiple copies of these. Especially given the problem that they may go stale if you carry them from one hook to another. I gave a link to an example in the comments showing that gcc is good at optimizing out references, even when they are class members.

I think it's better to have copies than a hidden lifetime dependency. The copies are cheap. Going stale is the same issue in either case - I don't see why it matters if the staleness is via reference or copy. In both cases all instances need to have been cleaned up.

Consider code like

MimeField host; if (some_flag) { HttpMsg msg = get_proxy_request(); host = msg.findMimeField("Host"); } else { HttpMsg msg = get_proxy_response(); host = msg.findMimeField("Host"); } field.assign(pristine_host); // Woops, field depends on an out of scope object.

Even with copies, your example is problematic. My current design is that ~HttpMsg() releases the TMLoc for the message. If it's not done in the destructor, then where? Why not:

HttpMsg msgReq = get_proxy_request(); HttpMsg msgRes = get_proxy_response(); MimeField host = (some_flag ? msgReq : msgResp).findMimeField("Host"); host.assign(pristine_host);

Each object tracks its own TSMLoc and releases it when destructed.

Your example could be simplified down to

MimeFile host = (some_flag ? get_proxy_request() : get_proxy_response()).findMimeField("Host"); host.assign(pristine_host);

Which causes the problem again. Certainly the reference style could be made to work, but it imposes additional very non-obvious requirements on how the classes can be used, for very little (if any) benefit.

I guess what you're saying is that MimeField doesn't get its MBuffer and TSMLoc instances by a simple assign, it gets new values that refer to the same buffer and message header. What is the TS API call to get these new values? Is there a TS API call that can check that two distinct MBuffer/TSMLoc instances refer to the same object?

Certainly there are advantages to having pointers be shared_ptr or something similar (or using some other form of garbage collection). But it's not the C++ way to use this approach exclusively. Rightly or wrongly, the perception is that the CPP API is too high-overhead. So that argues against increasing overhead to make it perfectly ironclad.

It looks like you are making multiple simple copies of the TSMLoc for the message headers: https://github.com/SolidWallOfCode/txn_box/blob/03093bf02e46f092691afe70d7b3d87151b2953a/plugin/include/txn_box/ts_util.h#L116 . Seems like your are leaking them.

OK looking at TSHandleMLocRelease(), it doesn't actually do anything for message header mlocs (HDR_HEAP_OBJ_HTTP_HEADER), are you saying we should rely on that fact?

For this function, https://github.com/SolidWallOfCode/txn_box/blob/03093bf02e46f092691afe70d7b3d87151b2953a/plugin/src/txn_box.cc#L122 , at what point does the returned TextView become stale?

BTW (some_flag ? get_proxy_request() : get_proxy_response()).findMimeField("Host") would not compile in my stuff because moving is deleted for the message classes.

include/tscpp/api/HttpMsgComp.h

ywkaras · 2019-08-14T22:39:57Z

@SolidWallOfCode for the "Transaction Box" stuff, do you have any gold tests that exercise it? If you're saying I should drop what I'm doing and work on Transaction Box instead, the problem is, it's not clear to me what all its goals are. I believe that a C++ utility, and yea even life itself, can still be worthwhile even if it uses neither std::tuple nor std::function.

scw00 · 2019-08-15T00:05:41Z

include/tscpp/api/HttpMsgComp.h

+  MimeField
+  newMimeField()
+  {
+    assert(hasMsg());


Why don't use the ink_assert or ink_release_assert ?

This header is exported to plugin code, so it has to be TSAssert(). See line 31, I removed the use of C assert. I verified that __OPTIMIZE__ is defined by both clang and gcc. The problem with using TSAssert (or ink_assert) directly is that (as in the check above) you also don't want any functions in the boolean expression called in the non-debug build. (Although, it this particular case, it wouldn't matter as hasMsg() would be inlined and elided by the optimizer.)

scw00 · 2019-08-15T00:10:01Z

include/tscpp/api/HttpMsgComp.h

+  bool
+  init(void *txnHndl)
+  {
+    assert(!hasMsg());


Seems obey the ATS Coding Style. See here https://cwiki.apache.org/confluence/display/TS/Coding+Style

Can you clarify? I don't find any mention of assert in the Coding Style.

SolidWallOfCode · 2019-08-15T15:54:59Z

TxnBox doesn't have any gold tests, but the equivalent classes are used extensively in the code so there is a lot of example use. I'm currently working on writing replay files for testing purposes.

The goals of TxnBox as a plugin aren't really relevant to this discussion. What is important from there is that it needs to interact with the C plugin API and wants to have C++ wrappers to do that, for which reason I have added code very similar to this. The point to address is, what are the goals of those C++ wrappers for the C plugin API? My version has been driven by the use cases that have arisen in TxnBox. For instance, there is a directive to set a field on the proxy request. How would that look with your utilities?

SolidWallOfCode · 2019-08-15T15:57:34Z

include/tscpp/api/HttpMsgComp.h

+
+  MimeField(MimeField &&source);
+
+  // Move assign only allowed if the two instances refer to the same message.


Why this restriction?

Because you can't change a reference.

Actually, you can, I do it, but it is a bit of a cheat.

Where do you do it?

ywkaras · 2019-08-15T16:52:50Z

@SolidWallOfCode It seems to me like what I'm doing is a thinner layer over the TS API HTTP message access functions. It's not immediately apparent to me what your goals are with the added complexity.

ywkaras · 2019-08-15T17:38:12Z

Setting a field to a single new value, in the general case, would look like:

MimeField f(msg, name);
if (f.valid()) {
  // Clear current values.
  MimeField fd = f.nextDup();
  while (fd.valid()) {
    f.destroy();
    f  = std::move(fd);
    fd = f.nextDup();
  }
  // Set single new value.
  f.clearValues();
  f.valAppend("New-Value");
}

SolidWallOfCode · 2019-08-20T15:55:58Z

I'm not sure what you mean by "added complexity". I think copying the TS C API values is a simpler implementation that using references across objects, so in my view your implementation has added complexity both internally and externally. My goal is to make the use of the classes as simple and robust as possible. Users shouldn't have to carefully design their use to accommodate internal implementation restrictions - the implementation should accommodate any reasonable use.

ywkaras · 2019-08-23T17:16:54Z

This is blocked by https://www.mail-archive.com/dev@trafficserver.apache.org/msg15104.html . Implementing this would allow the message objects to be copyable, which @SolidWallOfCode thinks is important.

ywkaras · 2020-06-16T16:40:38Z

Note: this is TS CPPAPI, not Core.

ywkaras · 2020-06-26T15:50:28Z

[approve ci autest]

ywkaras · 2020-06-26T15:52:23Z

tests/gold_tests/pluginTest/msg_comp/msg_comp.test.py

+#  limitations under the License.
+
+Test.Summary = '''
+Test tscpp/api/MsgComp.h:w


ywkaras · 2020-06-26T15:54:58Z

tests/gold_tests/pluginTest/msg_comp/msg_comp.test.py

+    'proxy.config.diags.debug.tags': 'test_msg_comp',
+})
+
+Test.PreparePlugin(Test.Variables.AtsTestToolsDir + '/plugins/test_msg_comp.cc', ts)


Have to change this to build in core since that's the New World Order.

ywkaras · 2020-06-26T16:16:24Z

tests/tools/plugins/test_msg_comp.cc

+
+#include <tscpp/api/HttpMsgComp.h>
+
+// TSReleaseAssert() doesn't seem to produce any logging output for a debug build, so do both kinds of assert.


This seems to work now so change to simply use TSReleaseAssert().

…TTP messagess. These classes may be used independently of other parts of the TS CPP API.

ywkaras · 2021-01-21T22:38:19Z

This PR is working properly, but marked as draft because we're not sure we want it.

…response body. Also add classes to CPP API for convenient manipulation of components of HTTP messagess. These classes may be used independently of other parts of the TS CPP API. (This PR is absorbing PR apache#5812.) Adds Au test with plugin forcing various errors on various hooks, with default body or set body.

ywkaras · 2022-02-10T16:39:24Z

As an alternative to HttpMsgComp.h, Alan proposes https://github.com/SolidWallOfCode/txn_box/blob/master/plugin/include/txn_box/ts_util.h https://github.com/SolidWallOfCode/txn_box/blob/master/plugin/src/ts_util.cc

bryancall assigned ywkaras Aug 9, 2019

ywkaras force-pushed the cppapi_msg branch 2 times, most recently from 2d7a00a to 514e60a Compare August 10, 2019 00:51

SolidWallOfCode reviewed Aug 12, 2019

View reviewed changes

ywkaras force-pushed the cppapi_msg branch 3 times, most recently from 9a45bdb to 46be42a Compare August 14, 2019 22:09

scw00 reviewed Aug 15, 2019

View reviewed changes

ywkaras force-pushed the cppapi_msg branch from 46be42a to ae28502 Compare August 15, 2019 00:18

SolidWallOfCode reviewed Aug 15, 2019

View reviewed changes

ywkaras force-pushed the cppapi_msg branch 4 times, most recently from 4bc5467 to 6d93bc5 Compare August 20, 2019 00:15

ywkaras force-pushed the cppapi_msg branch from 6d93bc5 to 5c3e438 Compare August 20, 2019 20:19

ywkaras force-pushed the cppapi_msg branch 2 times, most recently from 7be5328 to e98399f Compare September 16, 2019 17:08

ywkaras changed the title ~~Add classes for convenient manipulation of components of HTTP messages.~~ Add classes for convenient manipulation in plugins of components of HTTP messages. Sep 20, 2019

ywkaras force-pushed the cppapi_msg branch from e98399f to f036178 Compare September 20, 2019 22:37

ywkaras marked this pull request as ready for review September 20, 2019 23:16

zwoop added this to the 10.0.0 milestone Nov 4, 2019

zwoop added the Core label Nov 4, 2019

ywkaras commented Jun 26, 2020

View reviewed changes

ywkaras marked this pull request as draft June 26, 2020 16:22

ywkaras added CPP API and removed Core labels Jun 26, 2020

ywkaras modified the milestones: 10.0.0, Sometime Aug 31, 2020

ywkaras force-pushed the cppapi_msg branch 3 times, most recently from a89efec to 626c6fb Compare January 9, 2021 02:49

ywkaras marked this pull request as ready for review January 9, 2021 03:20

Add classes to CPP API for convenient manipulation of components of H…

d3dca64

…TTP messagess. These classes may be used independently of other parts of the TS CPP API.

ywkaras force-pushed the cppapi_msg branch from 626c6fb to d3dca64 Compare January 15, 2021 20:39

ywkaras marked this pull request as draft January 21, 2021 22:29

ywkaras added the Dormant label May 24, 2021

ywkaras closed this May 24, 2021

zwoop removed this from the Sometime milestone Sep 23, 2021

ywkaras removed the Dormant label Feb 4, 2022

ywkaras mentioned this pull request Feb 4, 2022

Make sure, when plugins cause a GET request to fail, there is always a response body. #8643

Closed

ywkaras added the Dormant label Feb 10, 2022

zwoop added Stale and removed Dormant labels Feb 3, 2023


		MimeField(MimeField &&source);

		// Move assign only allowed if the two instances refer to the same message.


		#include <tscpp/api/HttpMsgComp.h>

		// TSReleaseAssert() doesn't seem to produce any logging output for a debug build, so do both kinds of assert.

Add classes for convenient manipulation in plugins of components of HTTP messages. #5812

Add classes for convenient manipulation in plugins of components of HTTP messages. #5812

Uh oh!

Conversation

ywkaras commented Aug 9, 2019

Uh oh!

SolidWallOfCode commented Aug 12, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywkaras Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ywkaras commented Aug 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywkaras Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode commented Aug 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywkaras commented Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ywkaras commented Aug 15, 2019

Uh oh!

SolidWallOfCode commented Aug 20, 2019

Uh oh!

ywkaras commented Aug 23, 2019

Uh oh!

ywkaras commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ywkaras commented Jun 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywkaras commented Jan 21, 2021

Uh oh!

ywkaras commented Feb 10, 2022

Uh oh!

Reviewers

Assignees

ywkaras Aug 20, 2019 •

edited

Loading

ywkaras Aug 15, 2019 •

edited

Loading

ywkaras commented Aug 15, 2019 •

edited

Loading

ywkaras commented Jun 16, 2020 •

edited

Loading