[ADDED] In-process connections #2360

neilalexander · 2021-07-13T11:42:36Z

This PR adds three things:

InProcessConn() function to Server which builds a net.Pipe to get a connection to the NATS server without using TCP sockets
DontListen option which tells the NATS server not to listen on the usual TCP listener
startupComplete channel, which is closed right before we start AcceptLoop, and readyForConnections will wait for it

The main motivation for this is that we have an application that can run either in a monolith (single-process) mode or a polylith (multi-process) mode. We'd like to be able to use NATS for both modes for simplicity, but the monolith mode has to be able to cater for a variety of platforms where opening socket connections either doesn't make sense (mobile) or just isn't possible (WASM). These changes will allow us to use NATS entirely in-process instead.

An accompanying PR nats-io/nats.go#774 adds support to the client side.

This is my first PR to this project so apologies in advance if I've missed anything obvious anywhere.

/cc @nats-io/core

ripienaar

love this, been keen to have something like this for quite some time :)

server/server.go

kozlovic

Thank you for the contribution!

kozlovic · 2021-07-13T15:50:45Z

server/opts.go

@@ -184,6 +184,7 @@ type Options struct {
 	ServerName            string        `json:"server_name"`
 	Host                  string        `json:"addr"`
 	Port                  int           `json:"port"`
+	DontListen            bool          `json:"dont_listen"`


Instead of adding a new option, I wonder if we could decide on a port that would disable listening. For instance, port set to 0 means that we use default port 4222. Setting to -1 means that we let OS pick a random free port. We could say anything negative lower than -1 (say -2) means disabled?

You don't have to update the PR for that, just wondering if that would be better or not and let's see what others think.

I suppose this could work, although it does create the risk of the API shape having more "magic numbers". I'll be happy to accept direction on this from whoever has strong enough opinions on it.

server/server.go

kozlovic

The changes look good to me. Let's see if other maintainers have an opinion on the DontListen vs special port number.

ripienaar · 2021-07-14T05:16:33Z

The magic ports seem a bit unclear and hard to discover to me, otoh we have too many settings already but I think a documented setting might be better.

neilalexander · 2021-07-14T08:30:10Z

Hm, ultimately it doesn't appear as if the ReadyForConnections solution works, I guess because none of the chk cases fire in a simple setup and therefore we return after the first 25ms pause, regardless of whether NATS is set up or not by that point.

It might make sense to re-add the startupComplete channel, close it right before the AcceptLoop would start and then wait for that in ReadyForConnections before checking the sockets.

neilalexander · 2021-07-14T08:36:26Z

I've gone ahead and done that. There's no new API surface this time — readyForConnections will instead wait for the startup channel to close, which will happen right before AcceptLoop will be called. I hope that's OK — it fixes the problem in my tests.

kozlovic

I had a go with this change and bringing the changes in the nats.go to create a quick test in the server package and my main concern is the behavior of the net.Pipe when one side closes the connection.

It appears that it results in internal pipe write timeout, which is not that good. For instance, I tried to connect with wrong user/password and the server would report a WriteDeadline exceeded (when trying to write the authorization violation error).

Without more testing (which is going to be a bit annoying since it would require changes from the client that I would not want to commit if we end-up rejecting the whole idea), I think we can't merge this PR.
If you could have some tests and figure out what the issue is with those time outs, then we could reconsider.

server/server.go

neilalexander · 2021-07-14T15:58:53Z

It appears that it results in internal pipe write timeout, which is not that good. For instance, I tried to connect with wrong user/password and the server would report a WriteDeadline exceeded (when trying to write the authorization violation error).

Looking at the source for net.Pipe, the only way it can return a os.ErrDeadlineExceeded is if a deadline has been set and not cleared or re-set afterwards. Otherwise it should return an io.ErrClosedPipe.

https://github.com/golang/go/blob/d8f348a589b3df4bb48636023be4ccf9ac96d307/src/net/pipe.go#L183-L188

I haven't made any changes to deadlines anywhere so wondering if there is some existing deadline being set elsewhere?

If you could have some tests and figure out what the issue is with those time outs, then we could reconsider.

Have you got an example or a specific test that I can look at? We've been experimenting with in-process conns using these commits in our codebase for the last couple of days and we have yet to see this. We'll ultimately need to abandon NATS if we can't figure out a way to perform in-process connections which would be a shame, as it's looking to be a good fit so far.

kozlovic · 2021-07-14T16:24:25Z

Have you got an example or a specific test that I can look at?

I was trying to connect with wrong username/password, so what I see is that the server blocks on flushOutbound() writing -ERR 'Authorization Violation'\r\n message (up to write deadline which default is 10 seconds) and the client blocks in writing the CONNECT + PING protocols in nats.go sendConnect() at this place: if err := nc.bw.writeDirect(cProto, pingProto); err != nil {.

So the server reads the CONNECT (not the PING yet), fails the client by trying to write -ERR .. to it and that blocks because the client is not reading. The connect process in the client is synchronous, that is, there is no read loop started yet.

So if I have the client sends only CONNECT and then PING later, the issue is the same, this time the client blocks writing PING because the server is also blocked writing -ERR (since client is not reading). Having buffered pipes may help, but to the point where buffer will be full. At least that may help pass the phase of CONNECT...

neilalexander · 2021-07-14T16:30:32Z

Lack of buffering is probably the crux of the issue here — net.Pipe doesn't buffer. As per the godoc:

Reads on one end are matched with writes on the other, copying data directly between the two; there is no internal buffering.

I suppose a custom implementation could be made which wraps net.Pipe with buffered readers/writers, but I suspect that probably only masks the issue and makes for an even more confusing implementation. Not entirely sure what the best answer here is really and we won't have seen it because we don't use the user/password authentication (there's really no point when we know that the connection is coming from within the process).

variadico · 2021-07-14T18:56:07Z

Hm, ultimately it doesn't appear as if the ReadyForConnections solution works

Just curious, what is failing? Is there a test that consistently fails? Sometimes tests can be flaky, so you may need to rerun them a few times.

For example, I did this 537a5bc
and got green tests https://github.com/nats-io/nats-server/actions/runs/1031222424

kozlovic · 2021-07-14T18:58:58Z

@variadico No, it was just that in the case of "DontListen" (part of this PR), ReadyForConnections() should ignore the client port, so I asked to add ok: s.listener != nil || opts.DontListen, but the issue was that user calling ReadyForConnections() in that case would return immediately with "ok", while the user wanted at least all of Server.Start() (minus AcceptLoop) to be done, hence the use of the startupComplete channel that is just closed prior to starting the AcceptLoop() (which is blocking).

kozlovic · 2021-07-15T17:48:56Z

@neilalexander We will discuss this PR internally and come up with a decision maybe later next week. Thanks again!

kozlovic

LGTM. Switching to Draft until we decide internally (so it does not get merged by mistake).

neilalexander · 2021-07-15T17:56:31Z

Thanks @kozlovic, will wait to hear back on a way forward. Hoping we can come up with something suitable — NATS is shaping up very well for us so far, we just need to be able to get it to work in obscure places like WASM as well as cross-process!

neilalexander · 2021-07-26T10:18:12Z

Is there any update on whether this was discussed further? We're optimistically hoping for good news. 😄

derekcollison · 2021-07-26T14:20:00Z

@kozlovic and I plan to discuss this week IIRC.

kozlovic · 2021-07-26T15:06:23Z

Actually beginning of next since a member of the team is on vacation this week and we wanted his inputs.

kozlovic · 2021-08-02T16:47:32Z

@neilalexander Sorry for the delay. We just had a discussion today about this, and we feel it is important and part of future kind of deployments, but we would need a bit more information. The wasm limitation may be temporary, are you following wasi developments?
What wasm environment are you planning on deploying this?

Finally, would you have 15/20 minutes to speak with @derekcollison? He would like to have a short discussion with you. Thanks!

derekcollison · 2021-08-02T17:09:33Z

We are starting to see more interest in this and we ourselves are very interested in the WASM/WASI ecosystem. Possible to get 20 minutes of your time to chat live via Zoom or GH?

neilalexander · 2021-08-02T17:49:29Z

Hey @kozlovic, @derekcollison — more than happy to have a chat. I'm in GMT but please feel free to drop me a mail at neilalexander at matrix.org (or, if you are Matrix natives, the same MXID) to arrange a time.

At a high level, our project is Dendrite, which is a Matrix homeserver implementation. We basically need to be able to run everywhere — within the web browser, on mobile devices and on servers alike. We see the in-process connections as a way of getting around the limitations with not being able to open arbitrary sockets with WASM (and this probably still holds true in WASI as of today, not sure?) and also not opening unnecessary sockets on mobile devices either. I can explain more certainly about what we're hoping to do on the call.

derekcollison · 2021-08-24T14:23:13Z

Neil,

Have not forgotten about you, promise.

neilalexander · 2021-10-07T16:46:05Z

Hey Derek,

Is there any update? Cheers!

derekcollison · 2021-10-07T21:42:33Z

Still very much on our list, just prioritizing KV and ObjectStore abstractions atm.

neilalexander · 2022-02-03T09:28:07Z

Hi all, is there any update on this?

derekcollison · 2022-02-03T15:04:26Z

Unfortunately nothing has changed atm.

Still a nice to have I think for us but not a high priority.

derekcollison · 2022-06-20T19:29:34Z

@neilalexander if you want to update this to the main branch (rebase etc). We can see if we can get this one in..

neilalexander · 2022-06-21T08:43:50Z

@derekcollison Have rebased against main!

derekcollison · 2022-06-21T17:14:29Z

Thanks! We still have the hang issues that were mentioned yes?

neilalexander · 2022-06-21T18:16:34Z

I’m not entirely sure what issues it might introduce, but we’ve been using this in Dendrite for months without any particular issues. Admittedly we don’t ever stop NATS or close the connection until the point that the entire process shuts down. We’ve also seen one or two cases of slow consumer but I suspect that’s more to do with our message sizes as we’ve also seen it when running NATS in a separate process via TCP.

With an “experimental” label attached it’s usable as-is, but you’d probably be better qualified to figure out if there’s anything I’m missing.

derekcollison · 2022-06-21T19:01:01Z

I think Ivan had mentioned some tests he was doing that could block on failed auth or something.

kozlovic

Would you mind rebasing again (sorry about that). There was a test failure and I tried to recycle it on Travis but it complains about the script not being found (was changed during the Go 1.18 PR). Thanks!

server/opts.go

…ne call after createClient

neilalexander · 2022-06-28T16:04:10Z

Have rebased again.

kozlovic · 2022-06-28T16:57:07Z

server/server.go

 	for time.Now().Before(end) {
-		s.mu.RLock()
-		chk["server"] = info{ok: s.listener != nil, err: s.listenerErr}
+		s.mu.Lock()


You need to change that to an RLock(). This has changed in main so that is causing issue now with your PR.

server/opts.go

kozlovic

LGTM

The "InProcess" change make readyForConnections() possibly return much faster than it used to, which could cause tests to fail. Restore the original behavior, but in case of DontListen option wait on the startupComplete gate. Also fixed some missing checks for leafnode connections. Signed-off-by: Ivan Kozlovic <ivan@synadia.com>

Rework startupComplete gate from PR #2360

…erver#2360)

neilalexander mentioned this pull request Jul 13, 2021

In-process connections nats-io/nats.go#774

Merged

ripienaar reviewed Jul 13, 2021

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

kozlovic requested changes Jul 13, 2021

View reviewed changes

kozlovic approved these changes Jul 13, 2021

View reviewed changes

kozlovic reviewed Jul 13, 2021

View reviewed changes

kozlovic requested changes Jul 14, 2021

View reviewed changes

server/server.go Show resolved Hide resolved

server/server.go Show resolved Hide resolved

kozlovic approved these changes Jul 15, 2021

View reviewed changes

kozlovic marked this pull request as draft July 15, 2021 17:50

neilalexander mentioned this pull request Jul 20, 2021

Add NATS JetStream support matrix-org/dendrite#1866

Merged

2 tasks

gedw99 mentioned this pull request Nov 15, 2021

image-host make file has minor bug for "build-go" target wasmCloud/examples#50

Closed

derekcollison mentioned this pull request Feb 12, 2022

embedding nats server #2866

Closed

neilalexander marked this pull request as ready for review June 21, 2022 08:43

kozlovic requested changes Jun 27, 2022

View reviewed changes

server/opts.go Show resolved Hide resolved

neilalexander added 6 commits June 28, 2022 17:02

Add InProcessConn, DontListen

e9abc58

Update comments (re. review)

90d7e00

Review comments @kozlovic

9190feb

Re-add startupComplete channel, make readyForConnections wait for it

5b04c49

Remainder of time after waiting for startupComplete, add waitgroup do…

ff696f0

…ne call after createClient

Commit properly

cedf08a

kozlovic requested changes Jun 28, 2022

View reviewed changes

Fix the lock

558293e

kozlovic approved these changes Jun 28, 2022

View reviewed changes

kozlovic changed the title ~~In-process connections~~ [ADDED] In-process connections Jun 28, 2022

kozlovic merged commit cb406ed into nats-io:main Jun 28, 2022

derekcollison added a commit that referenced this pull request Jun 29, 2022

Merge pull request #3225 from nats-io/rework_starup_complete_check

88d5062

Rework startupComplete gate from PR #2360

neilalexander added a commit to neilalexander/nats.go that referenced this pull request Jul 30, 2022

Support for in-process connections (client support for nats-io/nats-s…

92c5e2e

…erver#2360)

kozlovic mentioned this pull request Dec 15, 2022

Unix socket for nats streaming. nats-io/nats-streaming-server#952

Closed

tpihl mentioned this pull request Mar 10, 2024

Embedded nats server with opts.DontListen=true fails to connect to cluster #5174

Open

[ADDED] In-process connections #2360

[ADDED] In-process connections #2360

Conversation

neilalexander commented Jul 13, 2021 • edited Loading

ripienaar left a comment

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

kozlovic Jul 13, 2021

Choose a reason for hiding this comment

neilalexander Jul 13, 2021

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

ripienaar commented Jul 14, 2021

neilalexander commented Jul 14, 2021

neilalexander commented Jul 14, 2021 • edited Loading

kozlovic left a comment

Choose a reason for hiding this comment

neilalexander commented Jul 14, 2021 • edited Loading

kozlovic commented Jul 14, 2021

neilalexander commented Jul 14, 2021

variadico commented Jul 14, 2021 • edited Loading

kozlovic commented Jul 14, 2021

kozlovic commented Jul 15, 2021

kozlovic left a comment

Choose a reason for hiding this comment

neilalexander commented Jul 15, 2021

neilalexander commented Jul 26, 2021

derekcollison commented Jul 26, 2021

kozlovic commented Jul 26, 2021

kozlovic commented Aug 2, 2021

derekcollison commented Aug 2, 2021

neilalexander commented Aug 2, 2021

derekcollison commented Aug 24, 2021

neilalexander commented Oct 7, 2021

derekcollison commented Oct 7, 2021

neilalexander commented Feb 3, 2022

derekcollison commented Feb 3, 2022

derekcollison commented Jun 20, 2022

neilalexander commented Jun 21, 2022

derekcollison commented Jun 21, 2022

neilalexander commented Jun 21, 2022 • edited Loading

derekcollison commented Jun 21, 2022

kozlovic left a comment

Choose a reason for hiding this comment

neilalexander commented Jun 28, 2022

kozlovic Jun 28, 2022

Choose a reason for hiding this comment

neilalexander Jun 28, 2022

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

neilalexander commented Jul 13, 2021 •

edited

Loading

neilalexander commented Jul 14, 2021 •

edited

Loading

neilalexander commented Jul 14, 2021 •

edited

Loading

variadico commented Jul 14, 2021 •

edited

Loading

neilalexander commented Jun 21, 2022 •

edited

Loading