Add Message Headers #394

ColinSullivan1 · 2020-07-10T01:34:46Z

Adding NATS message headers to parallel nats-io/nats.go#560.

Also see https://github.com/nats-io/nats-server/blob/master/doc/adr/0004-nats-headers.md

This also includes a change in CI testing to pull the NATS server from a dependency repository. This allows for testing against unreleased edge features.

Signed-off-by: Colin Sullivan <[email protected]>

watfordgnf

A few suggestions and comments.

src/NATS.Client/Info.cs

watfordgnf · 2020-07-10T12:42:18Z

src/NATS.Client/Msg.cs

+        internal static readonly string Header = "NATS/1.0\r\n";
+        internal static readonly byte[] HeaderBytes = Encoding.UTF8.GetBytes(Header);
+        internal static readonly int    HeaderLen = HeaderBytes.Length;
+        internal static readonly int    MinimalValidHeaderLen = Encoding.UTF8.GetBytes(Header + "k:v\r\n\r\n").Length;


Does this mean there is always a value?

Good point... while there's some debate it looks like some HTTP header implementations will support empty values - we should as well. I'll update and test for that. Thanks.

src/NATS.Client/Msg.cs

watfordgnf · 2020-07-10T12:49:50Z

src/NATS.Client/Parser.cs

@@ -21,6 +21,7 @@ internal sealed class MsgArg
 	    internal string subject;
 	    internal string reply;
 	    internal long   sid;
+        internal int    hdr;


Suggested change

internal int hdr;

internal int hdr;

* Performance updates * Avoid warning in test API usage * Upgrade test packages * Require .NET 4.6 to use the latest NameValueCollection class Signed-off-by: Colin Sullivan <[email protected]>

Signed-off-by: Colin Sullivan <[email protected]>

ColinSullivan1 · 2020-07-19T18:27:12Z

I think this should be good to go, would appreciate another quick pass @watfordgnf.

I also addressed the performance issues with Splits. Had to move up to .NET 4.6 to use the NameValueCollection class - while documentation says it's supported in 4.5, the assembly requires 4.6.

IMO it should be OK to move to 4.6 as the oldest supported framework version.

kozlovic

I don't think you can get away with a simple map. You need to extend to make sure that keys are stored/retried in their canonical form.
Support for multiple values also means that key "Foo" may return an array of ["value1", "value2"] and not a single string of "value1, value2".
Finally, it is possible to receive a multiple line value. So this:

NATS/1.0\r\nTwo-Lines: this is\r\n a multilines value\r\n\r\n

needs to become "Tow-Lines": "this is a multilines value". Notice that the value spreads on 2 lines and start with a space:" a multilines..." the space will then be removed a concatenated to the previous line.

kozlovic · 2020-07-20T17:04:49Z

src/Tests/UnitTests/TestMessageHeaders.cs

+            mh.Add("foo", "bar");
+            mh.Add("foo", "baz");
+
+            Assert.Equal("bar,baz", mh["foo"]);


I don't think this is correct. If there are 2 keys in the headers, you should return an array of strings, not a concatenated, comma separated, string.

I've updated the example documentation and tests, look for string[] values = mh.GetValues("foo");

src/NATS.Client/Msg.cs

ColinSullivan1 · 2020-07-20T17:59:49Z

@kozlovic , great points - thanks!

Because MsgHeader is a subclass of NameValueCollection, one could use the public virtual string[] GetValues (string name); API to return the values as a string array. The NameValueCollection class is a superclass for the .NET HTTP header classes - the string accessor returning comma separated values is an interesting feature that I used as a shortcut in testing.

I'll add tests for the GetValues APIs to be sure it works and will make sure multiline keys and values are handled correctly.

ColinSullivan1 · 2020-07-20T18:01:41Z

I can also add checks to make sure the keys are in canonical form. The server handled the tests fine though...

kozlovic · 2020-07-20T18:17:14Z

I can also add checks to make sure the keys are in canonical form. The server handled the tests fine though...

Because in most cases it does not do anything with the headers. But there is currently 1 case where the server will read headers, it is for JS publish de-dup and in that case it will look for "Msg-Id" and so if the user sets to "msg-id" and library doesn't use canonical form, then server won't de-dup.

ripienaar · 2020-07-20T18:26:34Z

yeah, server doesnt follow http spec, so gotta be careful. Also soon hopefully we'll add headers for tracing. I dont know .net but since you mentioned a hash map I'll also say that order of headers cant change - technically only the order of the same header more than once - but in general its probably a good idea not to tweak order

Signed-off-by: Colin Sullivan <[email protected]>

ColinSullivan1 · 2020-08-01T00:04:29Z

FYI, I've made various updates to cover what HTTP headers do, improve performance, and demonstrate multiple values in the documentation example and tests.

Signed-off-by: Colin Sullivan <[email protected]>

ColinSullivan1 · 2020-08-11T22:29:20Z

Are we g2g?

watfordgnf

Saw one fix and had some minor comments.

watfordgnf · 2020-08-12T13:36:13Z

src/NATS.Client/Conn.cs

@@ -2003,7 +2009,7 @@ private int setMsgArgsAryOffsets(byte[] buffer, long length)
            int i = 0;

            // We only support 4 elements in this protocol version


Is this still 4?

watfordgnf · 2020-08-12T13:38:40Z

src/NATS.Client/Msg.cs

+        }
+
+        // buffer for parsing strings
+        public char[] stringBuf = new char[64];


This should likely be internal.

Oof - thanks - will fix!

Signed-off-by: Colin Sullivan <[email protected]>

watfordgnf

Looks good.

ColinSullivan1 · 2020-08-13T17:59:25Z

Thanks! Good catch on the size of the args, appreciate it. Turns out I didn't resize the array - embarrassing, but would have been much more so after a release. :)

Signed-off-by: Colin Sullivan <[email protected]>

ColinSullivan1 added 5 commits July 9, 2020 17:41

use dependency repository

d28d185

Signed-off-by: Colin Sullivan <[email protected]>

add message headers

b141360

Signed-off-by: Colin Sullivan <[email protected]>

Adjust naming

973e9ae

Signed-off-by: Colin Sullivan <[email protected]>

Header naming

e7af70d

Signed-off-by: Colin Sullivan <[email protected]>

Fix naming

a482f77

Signed-off-by: Colin Sullivan <[email protected]>

ColinSullivan1 changed the title ~~Add Message Headers~~ Add Message Headers - WIP Jul 10, 2020

watfordgnf reviewed Jul 10, 2020

View reviewed changes

ColinSullivan1 added 9 commits July 17, 2020 14:12

Various updates based on comments

18730fa

* Performance updates * Avoid warning in test API usage * Upgrade test packages * Require .NET 4.6 to use the latest NameValueCollection class Signed-off-by: Colin Sullivan <[email protected]>

Updates for .NET 4.6

1f76220

Signed-off-by: Colin Sullivan <[email protected]>

fix clone command

9eb141a

Signed-off-by: Colin Sullivan <[email protected]>

remove unused vars

e112a64

Signed-off-by: Colin Sullivan <[email protected]>

update nuget and force restorewq

195e93b

Signed-off-by: Colin Sullivan <[email protected]>

pipeline fixes

ee53cb0

Signed-off-by: Colin Sullivan <[email protected]>

debugging pipelines

ae2e8cd

Signed-off-by: Colin Sullivan <[email protected]>

debugging pipelines

c71e4c1

Signed-off-by: Colin Sullivan <[email protected]>

cleanup

084e49d

Signed-off-by: Colin Sullivan <[email protected]>

ColinSullivan1 changed the title ~~Add Message Headers - WIP~~ Add Message Headers Jul 19, 2020

ColinSullivan1 requested a review from kozlovic July 19, 2020 18:21

kozlovic requested changes Jul 20, 2020

View reviewed changes

ColinSullivan1 mentioned this pull request Jul 22, 2020

Add support for no responders error on requests #400

Closed

performance and various updates

a7f7728

Signed-off-by: Colin Sullivan <[email protected]>

jasper-d mentioned this pull request Aug 3, 2020

Fix sync/async deadlock #404

Merged

add detail to flapping test

77c467d

Signed-off-by: Colin Sullivan <[email protected]>

ColinSullivan1 requested a review from watfordgnf August 11, 2020 22:29

watfordgnf reviewed Aug 12, 2020

View reviewed changes

Updates based on comments

8004d48

Signed-off-by: Colin Sullivan <[email protected]>

watfordgnf approved these changes Aug 13, 2020

View reviewed changes

Merge branch 'master' into headers, fix flapping test

6544249

Signed-off-by: Colin Sullivan <[email protected]>

ColinSullivan1 merged commit 50cb20d into master Aug 13, 2020

ColinSullivan1 deleted the headers branch August 13, 2020 19:13

This was referenced Aug 20, 2020

Add Message Headers nats-io/nats.java#335

Closed

Generic Header Support #391

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Message Headers #394

Add Message Headers #394

ColinSullivan1 commented Jul 10, 2020

watfordgnf left a comment

watfordgnf Jul 10, 2020

ColinSullivan1 Jul 10, 2020

watfordgnf Jul 10, 2020

ColinSullivan1 commented Jul 19, 2020

kozlovic left a comment

kozlovic Jul 20, 2020

ColinSullivan1 Aug 1, 2020

ColinSullivan1 commented Jul 20, 2020

ColinSullivan1 commented Jul 20, 2020

kozlovic commented Jul 20, 2020

ripienaar commented Jul 20, 2020 •

edited

Loading

ColinSullivan1 commented Aug 1, 2020

ColinSullivan1 commented Aug 11, 2020

watfordgnf left a comment

watfordgnf Aug 12, 2020

watfordgnf Aug 12, 2020

ColinSullivan1 Aug 12, 2020

watfordgnf left a comment

ColinSullivan1 commented Aug 13, 2020

		@@ -2003,7 +2009,7 @@ private int setMsgArgsAryOffsets(byte[] buffer, long length)
		int i = 0;

		// We only support 4 elements in this protocol version

Add Message Headers #394

Add Message Headers #394

Conversation

ColinSullivan1 commented Jul 10, 2020

watfordgnf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColinSullivan1 commented Jul 19, 2020

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColinSullivan1 commented Jul 20, 2020

ColinSullivan1 commented Jul 20, 2020

kozlovic commented Jul 20, 2020

ripienaar commented Jul 20, 2020 • edited Loading

ColinSullivan1 commented Aug 1, 2020

ColinSullivan1 commented Aug 11, 2020

watfordgnf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

watfordgnf left a comment

Choose a reason for hiding this comment

ColinSullivan1 commented Aug 13, 2020

ripienaar commented Jul 20, 2020 •

edited

Loading