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

net-lib: add net/server module with start-server and run-server #4621

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Bogdanp
Copy link
Sponsor Contributor

@Bogdanp Bogdanp commented Mar 31, 2023

Opening as draft in case folks have comments. If net-lib seems like a bad place for this, I can just make a new package, but it would be nice to have something like this in the main distribution.

This is a port of run-server from mzlib/thread, with some improvements carried over from changes I had made to that implementation when I ported it to web-server-lib. Inspired by a recent discussion on Discord.

This implementation should be compatible with the one in mzlib/thread so a follow-up change could be to rebase that implementation on top of this one, if merged, since compatibility-lib already depends on net-lib. It should also be possible to make web-server-lib use this implementation.

cc @LiberalArtist @samth

Copy link
Contributor

@LiberalArtist LiberalArtist left a comment

Choose a reason for hiding this comment

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

So glad you're working on this! Here are a few (well, more than a few …) initial thoughts.

Comment on lines 76 to 78
same meaning as in @racket[tcp-listen]. The @racket[listen],
@racket[accept] and @racket[close] arguments may be used to run the
server on an alternate protocol (such as SSL). The
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not confident that I have the details right, but I remember there being some subtleties here.

It looks like listen, accept, and close serve a similar role to the #:tcp@ argument to functions like serve from web-server/web-server. Those docs say:

Beware that the server expects the tcp-accept operation from tcp@ to be effectively atomic; new connections are not accepted while tcp-accept is in progress.

I can't find this explicitly written down anywhere right now, and it was long before I encountered Racket, but I think performance issues related to that caveat were the reason why this argument "was formerly recommended for SSL support." The current SSL/TLS support uses the #:dispatch-server-connect@ argument to provide an implementation of dispatch-server-connect^, particularly via make-ssl-connect@: basically it's a unit-based abstraction over something like ports->ssl-ports.

I think we should probably document this caveat and give examples both of good uses for listen/accept/close (racket/udp? racket/unix-socket? testing via Racket-level ports?) and of best practices for something like ports->ssl-ports.

@jeapostrophe may know the historical context and might be interested in this regardless.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Would it be worth it to build SSL handling in directly to this abstraction? Or is that adding too much complexity here?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I've added an example of using start-server with ports->ssl-ports and dropped the "(such as SSL)" bit from the documentation. I think in light of how simple ports->ssl-ports makes this, it's not worth adding explicit support here.

[port (integer-in 0 65535)]
[handle (-> input-port? output-port? any)]
[#:reuse? reuse? any/c #t]
[#:max-allow-wait max-allow-wait exact-nonnegative-integer? 511]
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember this seeming odd when I was working on make-unlimited-safety-limits: the contract from tcp-listen requires an exact-nonnegative-integer?, so there's no way to to specify "unlimited".

I chased this a little further just now, and I was further surprised to find that rktio_listen is actually passed (min max-allow-wait 10000):

(define lnr (rktio_listen rktio addr (min max-allow-wait 10000) reuse?))

Ultimately it becomes the backlog argument to int listen(int sockfd, int backlog) from sys/socket.h:

if (!listen(s, backlog)) {

My local man page says:

The behavior of the backlog argument on TCP sockets changed with Linux
2.2. Now it specifies the queue length for completely established
sockets waiting to be accepted, instead of the number of incomplete
connection requests. The maximum length of the queue for incomplete
sockets can be set using /proc/sys/net/ipv4/tcp_max_syn_backlog. When
syncookies are enabled there is no logical maximum length and this set‐
ting is ignored. See tcp(7) for more information.

If the backlog argument is greater than the value in
/proc/sys/net/core/somaxconn, then it is silently capped to that value.
Since Linux 5.4, the default in this file is 4096; in earlier kernels,
the default value is 128. In kernels before 2.4.25, this limit was a
hard coded value, SOMAXCONN, with the value 128.

If other systems are similarly accommodating about any system limits they impose on backlog, it seems Racket could expose an as-close-to-unlimited-as-possible value by passing INT_MAX.

In any case, I'm not sure where 511 originally came from—I used it in make-unlimited-safety-limits because it was the existing default for various web-server functions—but I'm skeptical that it makes sense for a general-purpose server framework.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Yes, I remember being baffled a year or two ago when I was profiling something and the backlog argument wasn't working as I expected. Finally, I opened the manual for listen and saw:

BUGS
The backlog is currently limited (silently) to 128.

I think I'll just change it so that it matches the default value of tcp-listen, namely 4.

pkgs/net-doc/net/scribblings/server.scrbl Outdated Show resolved Hide resolved
pkgs/net-doc/net/scribblings/server.scrbl Outdated Show resolved Hide resolved
pkgs/net-doc/net/scribblings/server.scrbl Outdated Show resolved Hide resolved
Comment on lines +75 to +76
The @racket[reuse?] and @racket[max-allow-wait] arguments have the
same meaning as in @racket[tcp-listen]. The @racket[listen],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm of two minds about these: handling them as arguments to start-server is obviously much more ergonomic for TCP, but I wonder how well they generalize to other kinds of connections.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, as I look more closely, it seems port, max-allow-wait, reuse?, and host are all used only by passing them to listen exactly once, before starting threads or looping or any of the other complexity:

  (define listener
    (listen port max-allow-wait reuse? host))

What would you think of dropping all of them and having start-server take the listener as an argument, instead?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Thanks for all the feedback so far!

What would you think of dropping all of them and having start-server take the listener as an argument, instead?

I'm reluctant to make that change, because it feels wrong for the listener to be created externally to start-server and for start-server to then take ownership of the listener and be in charge of closing it. My preference would be to reduce the set of arguments that start-server takes and rely on the caller to provide a customized listen-proc.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

After working on the new set of examples, I think it's probably worth keeping the keywords as-is. Partly because TCP is likely to be the common use case, but also because of the implementation of tcp-listen: the host argument is the final argument, so we'd have to provide our own defaults to listen-proc anyway (which is what mzlib's run-server does, but seems worse). Having the host argument be a required arg to start-server seems better than relying on tcp-listen's default of listening on all interfaces, too.

Comment on lines 86 to 111
To listen on and retrieve an ephemeral port, set the @racket[port]
argument to @racket[0] and pass a custom @racket[listen] procedure
that retrieves the port using @racket[tcp-addresses] in the case of
a TCP server. For example:

@examples[
#:label #f
#:eval ev
(require net/server
racket/tcp)
(define ((make-listen-proc port-ch) port backlog reuse? host)
(define listener
(tcp-listen port backlog reuse? host))
(define-values (local-host local-port remote-host remote-port)
(tcp-addresses listener #t))
(thread (lambda () (channel-put port-ch local-port)))
listener)
(define port-ch
(make-channel))
(define stop
(start-server
#:listen-proc (make-listen-proc port-ch)
"127.0.0.1" 0 void))
(channel-get port-ch)
(stop)
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Having run-server take listener as an argument would simplify this considerably.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This was also my major reservation here. The make-listen-proc pattern here seems like it could usefully be a part of the abstraction.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

My argument here would be that this is a special case. Most users of these procedures are likely not going to be using ephemeral ports and I wouldn't have used them here if I could know for sure that the documentation would build cleanly with specific ports used in the examples. Probably, I should use a log-based evaluator instead for the simple examples and mark this example as a special case.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

That sounds like a sensible plan; I think we want the examples to be what people would actually do.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

OK, I've made a separate section for examples and added TCP, TCP+SSL, Unix socket and port examples and dropped the ephemeral port example entirely.

pkgs/net-lib/net/server.rkt Outdated Show resolved Hide resolved
Comment on lines 95 to 96
(procedure-rename
(procedure-reduce-keyword-arity
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're using procedure-reduce-keyword-arity anyway, you don't need procedure-rename: you can just pass the name to procedure-reduce-keyword-arity. Also, it might be slightly more efficient to use procedure-reduce-keyword-arity-mask and procedure-arity-mask.

#:max-concurrent (or/c +inf.0 exact-positive-integer?)
#:listen-proc (-> listen-port-number? exact-nonnegative-integer? any/c (or/c #f string?) evt?)
#:accept-proc (-> any/c (values input-port? output-port?))
#:close-proc (-> any/c void?)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we probably don't want to enforce that close-proc specifically return void? rather than any/c. Actually, it seems like (-> any/c any) would work fine.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I prefer to enforce since typically these kinds of destructive procedures return void. We can relax it later if necessary, but I think it's more likely to catch issues as is.

[#:reuse? reuse? any/c #t]
[#:max-allow-wait max-allow-wait exact-nonnegative-integer? 4]
[#:max-concurrent max-concurrent
(or/c +inf.0 exact-positive-integer?)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should accept (or/c +inf.0 natural-number/c). It would admittedly be a bit silly to start a server that would never accept any connections, but maybe it would be useful for testing or something.

Comment on lines +28 to +31
#:listen-proc [listen tcp-listen]
#:accept-proc [accept tcp-accept]
#:close-proc [close tcp-close]
#:timeout-evt-proc [make-timeout-evt (λ (_thd _in _out _break-sent?) never-evt)])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd rather the keywords be #:listen, #:accept, and #:close. I'd also prefer #:make-timeout-evt to #:timeout-evt-proc, but maybe there's something even better.

#:max-allow-wait [max-allow-wait 4]
#:max-concurrent [max-concurrent +inf.0]
#:listen-proc [listen tcp-listen]
#:accept-proc [accept tcp-accept]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need an argument like tcp-accept/enable-break? I just saw that the old run-server had both: I haven't refreshed my memory as to why yet.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Not with this implementation, since the job of allowing breaks within the listening thread is handled by the sync/enable-break call instead.

@LiberalArtist
Copy link
Contributor

Moving down here to reply to a few threads at once.

Re #4621 (comment):

After working on the new set of examples, I think it's probably worth keeping the keywords as-is. Partly because TCP is likely to be the common use case, but also because of the implementation of tcp-listen: the host argument is the final argument, so we'd have to provide our own defaults to listen-proc anyway (which is what mzlib's run-server does, but seems worse). Having the host argument be a required arg to start-server seems better than relying on tcp-listen's default of listening on all interfaces, too.

From my perspective, I think the essence of this abstraction is in managing the subtleties of threads (listening thread, connection-handling threads, and supervisor threads), custodians, exceptions, parameterizations, breaks, and graceful (or forcible) shutdown: many of the ideas presented in More: Systems Programming with Racket. (I think it would be worth adding a link to net/server from its conclusion!)

We definitely want to provide an excellent experience with TCP, since I indeed expect that will be the most common use case. Even in the relatively near term, though, I could imagine QUIC supplanting many uses for TCP+TLS if we had it implemented in Racket, and similarly I expect at least some of TCP's current popularity in Racket is because we don't have much support for named pipes (without resorting to extra-linguistic mechanisms) and make-unix-socket-tcp@ is easy to overlook. (This reminds me that I've meant to look at racket/unix-socket#6 some day.)

I think we agree that one of the reasons the old run-server deserved to be deprecated is that it baked a few two many assumptions in at the wrong level (I particularly remember the kill timeout being twice the break timeout), such that the web server's use of it involved passing dummy values for certain arguments and working around some of its behavior. I think a goal for net/server should be not to require such hijinks for protocols that might be considerably different than TCP.

Re #4621 (comment):

What would you think of dropping all of them and having start-server take the listener as an argument, instead?

I'm reluctant to make that change, because it feels wrong for the listener to be created externally to start-server and for start-server to then take ownership of the listener and be in charge of closing it. My preference would be to reduce the set of arguments that start-server takes and rely on the caller to provide a customized listen-proc.

I'd see this as similar to reencode-input-port, port->string, and many other functions from racket/port which which accept a close? argument. In particular, passing #:close void to start-server would work like #:close? #f in those cases.

I do agree that supporting some arguments tied to tcp-listen while also relying on some defaults seems like a particularly unpleasant option, but I'm increasingly interested in a signature like this:

(define (start-server listener handle
                      #:max-concurrent [max-concurrent +inf.0]
                      #:accept [accept tcp-accept]
                      #:close [close tcp-close]
                      #:make-timeout-evt
                      [make-timeout-evt (λ (_thd _in _out _break-sent?)
                                          never-evt)])
  ...)

It also has the benefit of clearly communicating which arguments control the functionality of start-server and distinguishing them from the general options for configuring a TCP listener.

Re #4621 (comment):

My argument here would be that this is a special case. Most users of these procedures are likely not going to be using ephemeral ports and I wouldn't have used them here if I could know for sure that the documentation would build cleanly with specific ports used in the examples. Probably, I should use a log-based evaluator instead for the simple examples and mark this example as a special case.

Actually, I thought this was a great example! It took me a long time to figure out how to learn what ephemeral port a listener had been assigned (I somehow missed #:confirmation-channel, or actually I guess serve/launch/wait doesn't support that option), and I wrote several programs with hard-coded ports when I really would have preferred ephemeral ones as a result.

Moreover, it prompted me to think more (and I'm continuing to do so) about what kinds of additional communication protocols other than TCP might want. Maybe QUIC Connection IDs? Or maybe not.

Comment on lines +47 to +52
(with-handlers ([exn:fail:network?
(λ (e)
(begin0 in-progress
((error-display-handler)
(format "Connection error: ~a" (exn-message e))
e)))])
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a place where it would be nice to allow interposition, though I'm not sure exactly how.

@Bogdanp
Copy link
Sponsor Contributor Author

Bogdanp commented Apr 2, 2023

I don't think I have any new thoughts regarding the external listener approach. I think passing in a listener breaks encapsulation in a way that makes me uncomfortable, but I don't have any better arguments against it. That approach does strictly limit what start-server can do (eg. it can't wrap the listener in its own custodian should we want to add a mode later on that guarantees it frees all resources on stop), but that's not a very strong argument. I think ergonomics of passing in a listener are also worse for the general case, but I've already said that.

I think a QUIC listener is likely to also take similar arguments, though maybe not in the same order, so probably a use would look like:

(define stop
  (start-server 
   #:listen-proc (lambda (port backlog reuse? host)
                   (quic-listen ssl-ctx host port backlog))
   #:accept-proc quic-accept
   #:close-proc quic-close
   host port handle))

While I agree that it's not ideal to have to shift arguments around and to outright ignore one of them, I still feel that's better, and probably less error prone, than:

(define listener (quic-listen ssl-ctx host port backlog))
(define stop
  (start-server 
   #:accept-proc quic-accept
   #:close-proc quic-close
   listener handle))

Because there's more of a disconnect between things in the 2nd example. I'm more likely to only pass in a listener and forget to pass in accept and close procs in the 2nd case. Of course, I could instead write

(define stop
  (start-server 
   #:accept-proc quic-accept
   #:close-proc quic-close
   (quic-listen ssl-ctx host port backlog) 
   handle))

and maybe that makes my last point moot, but then again that might also mean I could write

(define stop
  (start-server (tcp-listen port) handle))

and not realize that my server is now binding to all interfaces. What's worse, now I have to write

(define stop
  (start-server (tcp-listen 8000 4 #f "127.0.0.1") handle))

just to get it to listen only on localhost. Versus:

(define stop
  (start-server "127.0.0.1" 8000 handle))

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.

None yet

3 participants