Skip to content

Commit

Permalink
RTR Server: thread-pool.server.max now refers to RTR requests
Browse files Browse the repository at this point in the history
Apparently, there was a huge misunderstanding when the thread pool was
implemented.

The intended model was

> When the RTR server receives a request, it borrows a thread from the
> thread pool, and tasks it with the request.

Which is logical and a typical thread pool use case. However, what was
actually implemented was

> When the RTR server opens a connection, it borrows a thread from the
> thread pool, and tasks it with the whole connection.

So `thread-pool.server.max` was a hard limit for simultaneous RTR
clients (routers), but now it's just a limit to simultaneous RTR
requests. (Surplus requests will queue.) This is much less taxing to the
CPU when there are hundreds of clients.

Thanks to Mark Tinka for basically spelling this out to me.

-----------------------

Actually, this commit is an almost entire rewrite of the RTR server
core. Here's a (possibly incomplete) list of other problems I had to fix
in the process:

== Problem 1 ==

sockaddr2str() was returning a pointer to invalid memory on success.

This was due to a naive attempt of a bugfix from
1ff403a.

== Problem 2 ==

Changed the delta expiration conditional.

Was "keep track of the clients, expire deltas when all clients outgrow
them." I see two problems with that:

1. It'll lead to bad performance if a client misbehaves by not
   maintaining the connection. (ie. the server will have to fall back to
   too many cache resets.)
2. It might keep the deltas forever if a client bugs out without killing
   the connection.

New conditional is "keep deltas for server.deltas.lifetime iterations."
"server.deltas.lifetime" is a new configuration argument.

== Problem 3 ==

Serials weren't being compared according to RFC 1982 serial arithmetic.
This was going to cause mayhem when the integer wrapped.

(Though Fort always starts at 1, and serials are 32-bit unsigned
integers, so this wasn't going to be a problem for a very long time.)

== Problem 4 ==

The thread pool had an awkward termination bug. When threads were
suspended, they were meant to be ended through a pthread signal, but
when they were running, they were supposed to be terminated through
pthread_cancel(). (Because, since each client was assigned a thread,
they would spend most of their time sleeping.) These termination methods
don't play well with each other.

Apparently, threads waiting on a signal cannot be canceled, because of
this strange quirk from man 3 pthread_cond_wait:

> a side effect of acting upon a cancellation request while in a
> condition wait is that the mutex is (in effect) re-acquired before
> calling the first cancellation cleanup handler.

(So the first thread dies with the mutex locked, and no other threads
can be canceled because no one can ever lock the mutex again.)

And of course, you can't stop a server thread through a signal, because
they aren't listening to it; they're sleeping in wait for a request.

I still don't really know how would I fix this, but luckily, the problem
no longer exists since working threads are mapped to single requests,
and therefore no longer sleep. (For long periods of time, anyway.)
So always using the signal works fine.
  • Loading branch information
ydahhrk committed Jul 13, 2021
1 parent bfb9970 commit 23478fd
Show file tree
Hide file tree
Showing 46 changed files with 1,495 additions and 1,743 deletions.
4 changes: 3 additions & 1 deletion src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ fort_SOURCES += address.h address.c
fort_SOURCES += algorithm.h algorithm.c
fort_SOURCES += certificate_refs.h certificate_refs.c
fort_SOURCES += cert_stack.h cert_stack.c
fort_SOURCES += clients.c clients.h
fort_SOURCES += clients.h
fort_SOURCES += common.c common.h
fort_SOURCES += config.h config.c
fort_SOURCES += daemon.h daemon.c
Expand All @@ -29,6 +29,7 @@ fort_SOURCES += random.h random.c
fort_SOURCES += reqs_errors.h reqs_errors.c
fort_SOURCES += resource.h resource.c
fort_SOURCES += rpp.h rpp.c
fort_SOURCES += serial.h serial.c
fort_SOURCES += sorted_array.h sorted_array.c
fort_SOURCES += state.h state.c
fort_SOURCES += str_token.h str_token.c
Expand Down Expand Up @@ -109,6 +110,7 @@ fort_SOURCES += rtr/rtr.c rtr/rtr.h

fort_SOURCES += rtr/db/db_table.c rtr/db/db_table.h
fort_SOURCES += rtr/db/delta.c rtr/db/delta.h
fort_SOURCES += rtr/db/deltas_array.c rtr/db/deltas_array.h
fort_SOURCES += rtr/db/roa.c rtr/db/roa.h
fort_SOURCES += rtr/db/vrp.h
fort_SOURCES += rtr/db/vrps.c rtr/db/vrps.h
Expand Down
22 changes: 12 additions & 10 deletions src/address.c
Original file line number Diff line number Diff line change
Expand Up @@ -518,15 +518,16 @@ ipv6_covered(struct in6_addr *f_addr, uint8_t f_len, struct in6_addr *son_addr)
/**
* buffer must length INET6_ADDRSTRLEN.
*/
char const *
sockaddr2str(struct sockaddr_storage *sockaddr)
void
sockaddr2str(struct sockaddr_storage *sockaddr, char *buffer)
{
void *addr = NULL;
char const *addr_str;
char buffer[INET6_ADDRSTRLEN];
char const *str;

if (sockaddr == NULL)
return "(null)";
if (sockaddr == NULL) {
strcpy(buffer, "(null)");
return;
}

switch (sockaddr->ss_family) {
case AF_INET:
Expand All @@ -536,11 +537,12 @@ sockaddr2str(struct sockaddr_storage *sockaddr)
addr = &((struct sockaddr_in6 *) sockaddr)->sin6_addr;
break;
default:
return "(protocol unknown)";
strcpy(buffer, "(protocol unknown)");
return;
}

addr_str = inet_ntop(sockaddr->ss_family, addr, buffer,
INET6_ADDRSTRLEN);
return (addr_str != NULL) ? addr_str : "(unprintable address)";
str = inet_ntop(sockaddr->ss_family, addr, buffer, INET6_ADDRSTRLEN);
if (str == NULL)
strcpy(buffer, "(unprintable address)");
}

2 changes: 1 addition & 1 deletion src/address.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,6 @@ int ipv6_prefix_validate(struct ipv6_prefix *);
bool ipv4_covered(struct in_addr *, uint8_t, struct in_addr *);
bool ipv6_covered(struct in6_addr *, uint8_t, struct in6_addr *);

char const *sockaddr2str(struct sockaddr_storage *);
void sockaddr2str(struct sockaddr_storage *, char *buffer);

#endif /* SRC_ADDRESS_H_ */
6 changes: 3 additions & 3 deletions src/asn1/asn1c/INTEGER.c
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ INTEGER__dump(const asn_TYPE_descriptor_t *td, const INTEGER_t *st, asn_app_cons
}

/* Output in the long xx:yy:zz... format */
/* TODO: replace with generic algorithm (Knuth TAOCP Vol 2, 4.3.1) */
/* TODO (asn1c) replace with generic algorithm (Knuth TAOCP Vol 2, 4.3.1) */
for(p = scratch; buf < buf_end; buf++) {
const char * const h2c = "0123456789ABCDEF";
if((p - scratch) >= (ssize_t)(sizeof(scratch) - 4)) {
Expand Down Expand Up @@ -684,7 +684,7 @@ INTEGER_decode_uper(const asn_codec_ctx_t *opt_codec_ctx,
/* #12.2.3 */
if(ct && ct->lower_bound) {
/*
* TODO: replace by in-place arithmetics.
* TODO (asn1c) replace by in-place arithmetics.
*/
long value = 0;
if(asn_INTEGER2long(st, &value))
Expand Down Expand Up @@ -779,7 +779,7 @@ INTEGER_encode_uper(const asn_TYPE_descriptor_t *td,

if(ct && ct->lower_bound) {
ASN_DEBUG("Adjust lower bound to %ld", ct->lower_bound);
/* TODO: adjust lower bound */
/* TODO (asn1c) adjust lower bound */
ASN__ENCODE_FAILED;
}

Expand Down
2 changes: 1 addition & 1 deletion src/asn1/asn1c/OCTET_STRING.c
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,7 @@ OS__check_escaped_control_char(const void *buf, int size) {
/*
* Inefficient algorithm which translates the escape sequences
* defined above into characters. Returns -1 if not found.
* TODO: replace by a faster algorithm (bsearch(), hash or
* TODO (asn1c) replace by a faster algorithm (bsearch(), hash or
* nested table lookups).
*/
for(i = 0; i < 32 /* Don't spend time on the bottom half */; i++) {
Expand Down
2 changes: 1 addition & 1 deletion src/asn1/asn1c/constr_SEQUENCE.c
Original file line number Diff line number Diff line change
Expand Up @@ -1472,7 +1472,7 @@ SEQUENCE_encode_uper(const asn_TYPE_descriptor_t *td,

ASN_DEBUG("Bit-map of %d elements", n_extensions);
/* #18.7. Encoding the extensions presence bit-map. */
/* TODO: act upon NOTE in #18.7 for canonical PER */
/* TODO (asn1c) act upon NOTE in #18.7 for canonical PER */
if(SEQUENCE__handle_extensions(td, sptr, po, 0) != n_extensions)
ASN__ENCODE_FAILED;

Expand Down
260 changes: 0 additions & 260 deletions src/clients.c

This file was deleted.

Loading

0 comments on commit 23478fd

Please sign in to comment.