Skip to content

Headers parsing#63

Merged
mtmk merged 11 commits intomainfrom
62-client-protocol-nats-headers-support
Jun 21, 2023
Merged

Headers parsing#63
mtmk merged 11 commits intomainfrom
62-client-protocol-nats-headers-support

Conversation

@mtmk
Copy link
Copy Markdown
Member

@mtmk mtmk commented Jun 19, 2023

Implementations of HPUB and HMSG messages.

Also includes minor code clean-up and some test improvements to reduce false positives.

@mtmk mtmk linked an issue Jun 19, 2023 that may be closed by this pull request
// PUB <subject> [reply-to] <#bytes>\r\n[payload]\r\n
public void WritePublish(string subject, string? replyTo, NatsHeaders? headers, ReadOnlySequence<byte> payload)
{
var offset = 0;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Removed precalculated offset +=... code since BufferWriter.Advance() method (which also marked with AggressiveInlining) does just that anyway and makes it much easier comprehend.


[Fact]
public async Task Subscription_with_same_subject()
{
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is moved to the ProtocolTest

@mtmk mtmk marked this pull request as ready for review June 20, 2023 19:09
@mtmk mtmk requested a review from caleblloyd June 20, 2023 19:09
{
Connection = connection;
Manager = manager;
HeaderParser = new HeaderParser(Encoding.UTF8);
Copy link
Copy Markdown
Collaborator

@caleblloyd caleblloyd Jun 20, 2023

Choose a reason for hiding this comment

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

Header Parser is thread safe, right? Should we allocate 1 per connection as opposed to 1 per subscription.

Also, the default encoding is technically ASCII since this follows the HTTP RFC

Certain clients enforce ASCII while others do not though. For example the Go implementation defaults to UTF-8.

So I think that we should add HeaderEncoding to NatsOptions, default it to Encoding.ASCII there but let someone change it if they want.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

All makes sense.

mtmk and others added 3 commits June 21, 2023 12:44
Co-authored-by: Caleb Lloyd <2414837+caleblloyd@users.noreply.github.com>
Co-authored-by: Caleb Lloyd <2414837+caleblloyd@users.noreply.github.com>
Also, test resilience improvements.
Copy link
Copy Markdown
Collaborator

@caleblloyd caleblloyd left a comment

Choose a reason for hiding this comment

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

Implementation LGTM - looks like a test might be flapping though because the CI failed

mtmk added 5 commits June 21, 2023 17:16
This was caused by recent nats-server fix.
GitHub CI seems to have slowed down!?
On my local ubuntu vm runs under a minute.
Separated server version read.
@mtmk
Copy link
Copy Markdown
Member Author

mtmk commented Jun 21, 2023

Implementation LGTM - looks like a test might be flapping though because the CI failed

CI was failing because of a change on the server reporting WS port instead of standard port which broke cluster reconnect test. There was also an issue reading the server version in tests. All issues in test code and they're fixed now.

@mtmk mtmk merged commit 7d0ecad into main Jun 21, 2023
@mtmk mtmk deleted the 62-client-protocol-nats-headers-support branch June 21, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Client protocol NATS headers support

2 participants