HTTP server accept throttle callback #578

vii · 2017-12-01T01:35:20Z

Add callback to allow HTTP servers to decline (and thereby close) incoming connections.

This is important, as otherwise clients can easily exhaust the file
descriptors available on a libevent HTTP server, which can cause
problems in other code which does not handle EMFILE well: for example,
see bitcoin/bitcoin#11368

coveralls · 2017-12-01T18:00:54Z

Coverage increased (+0.04%) to 80.602% when pulling 13885ab on vii:master into c2c08e0 on libevent:master.

coveralls · 2017-12-01T18:00:54Z

Coverage increased (+0.04%) to 80.602% when pulling 13885ab on vii:master into c2c08e0 on libevent:master.

vii · 2017-12-04T06:00:21Z

I notice this failure on one configuration - however, all the other HTTP tests seem to have failed too? Is there anything I can do?

 FAIL test/regress_http.c:4620: assert(state->connections_throttled >= state->connections_error): 15 vs 16http/throttlereqcb: 
  FAIL test/regress_http.c:4631: (Failed.)
  [throttlereqcb FAILED]
FAILED

azat

Sorry for delay, could you take a look at the comments?

azat · 2017-12-11T21:43:29Z

http.c

 	    case 3:
 		/* The length of the method string is 3, meaning it can only be one of two methods: GET or PUT */
-            
+


Please clean this patch from this whitespace fixes (I know I know, it looks better now, but it pollutes the patch)

azat · 2017-12-11T21:45:23Z

include/event2/http.h

+
+/* Older versions of libevent did not have this callback, so make it
+ * possible for users to detect it */
+#define EVENT2_HTTP_HAS_THROTTLECB


I don't like it here, we should either move it into some *-config.h or just let user detect this by himself, using his build system (well it is simpler to just #ifdef but we don't have such defines for other functions). Thoughts?

plus I guess it should be defined as EVENT2_HTTP_HAS_THROTTLEREQCB to avoid different namings

I guess this should be renamed with the other rename.

My suggestion here is that we should make presence of this callback easy to detect at runtime.

But this is not "at runtime", and there is nothing that we can do here

azat · 2017-12-11T21:46:53Z

include/event2/http.h

 	const char *address, ev_uint16_t port);

-/**
+/**	


Well this is definitely a bad hunk (even though previous whitespace hunks were fine, at least they make sense)

azat · 2017-12-11T21:49:36Z

listener.c

 	void *user_data;
 	LOCK(lev);
-	while (1) {
+	while (lev->enabled) {


what this for? even though this makes sense, this should be a separate patch, otherwise it will became too tricky to track changes

the intention was to immediately act, if a cb decides to disable the listener, to prevent it draining the queue.

If you would accept this as a separate PR I would add it

yep, just submit another PR, since this is totally different thing

azat · 2017-12-11T21:50:24Z

test/regress_http.c

+	return;
+end:
+	tt_fail();
+	exit(17);


I guess that tt_abort() is better instead, no? or does the semantic differs?

tt_abort ends up doing goto end - I copied this more drastic exit from elsewhere in the file

azat · 2017-12-11T21:52:17Z

test/regress_http.c

+	struct http_throttlereqcb_test_state* state = arg;
+	state->connections_noticed ++;
+	http_throttlereqcb_test_state_check(state);
+	if (1 == state->connections_noticed%7) {


ugh, why do people prefer 1 == some_var instead of some_var == 1, never mind.

fewer typos!

sorry, don't see your point

azat · 2017-12-11T21:56:06Z

http.c

 }

+void
+evhttp_set_throttlereqcb(struct evhttp *http,


maybe this could be renamed, since this is useful not only for throttling, but also for some kind of whitelist.

@azat exactly - happy to rename it, what would you like me to call it?

@azat called it newreqcb

Yep, sounds more generic

azat · 2017-12-11T21:56:43Z

http.c


-	TAILQ_INSERT_TAIL(&evcon->requests, req, next);
+	if (http->throttlereqcb) {
+	        if (http->throttlereqcb(req, http->throttlereqcbarg)) {


whitespace issues, we use tabs, not spaces

(and please check all other places)

plus I guess this can be rewritten with if (http->throttlereqcb && http->throttlereqcb(...)) { ... }

done! Hope the whitespace is better now!

azat · 2017-12-11T22:00:06Z

test/regress_http.c

+http_request_error_throttlereqcb(enum evhttp_request_error err, void *arg)
+{
+	struct http_throttlereqcb_test_state* state = arg;
+	state->connections_error ++;


what space for?

(and in other places too)

azat · 2017-12-11T22:01:10Z

test/regress_http.c

+ end:
+	evhttp_free(http);
+
+	for (n = 0; n < sizeof(evcons)/sizeof(evcons[0]); ++n) {


there is ARRAY_SIZE() in regress_dnc.c, could be moved into some header and re-used here

would that really be clearer? People unfamiliar with the project would have to understand which kind of size is being referred to by that macro

This is pretty common macro I suppose, but I'm fine with that sizeof/sizeof

azat · 2017-12-11T22:07:26Z

I notice this failure on one configuration - however, all the other HTTP tests seem to have failed too? Is there anything I can do?

could give the link to this failures?

vii · 2017-12-12T06:44:30Z

the failures I am referring to are in this run https://travis-ci.org/libevent/libevent/jobs/309815275

Sorry for the whitespace changes. I'll try to go through and fix them.

coveralls · 2017-12-16T22:02:28Z

Coverage increased (+0.05%) to 80.528% when pulling 1e1f699 on vii:master into 65eb529 on libevent:master.

coveralls · 2017-12-16T22:02:28Z

Coverage increased (+0.05%) to 80.528% when pulling 1e1f699 on vii:master into 65eb529 on libevent:master.

azat · 2017-12-16T22:45:41Z

Sorry for the whitespace changes. I'll try to go through and fix them.

yep, now it looks cleaner, thanks!

azat · 2017-12-16T22:49:48Z

the failures I am referring to are in this run https://travis-ci.org/libevent/libevent/jobs/309815275

grep connections_throttled doesn't show anything, and your tests passes there, while you reported such trace before:

 FAIL test/regress_http.c:4620: assert(state->connections_throttled >= state->connections_error): 15 vs 16http/throttlereqcb: 
  FAIL test/regress_http.c:4631: (Failed.)
  [throttlereqcb FAILED]
FAILED

azat · 2017-12-16T22:52:07Z

include/event2/http.h

+ */
+EVENT2_EXPORT_SYMBOL
+void evhttp_set_newreqcb(struct evhttp *http,
+    int (*cb)(struct evhttp_request*, void *), void *arg);


I was thinking about returning enum instead, but don't have a clear idea what we can add in future, plus all other callbacks returns int. At least let's make well-defined return code, let's say "If the callback returns -1, the connection will be terminated"

coveralls · 2017-12-17T01:32:46Z

Coverage increased (+0.05%) to 80.527% when pulling 1e1f699 on vii:master into 65eb529 on libevent:master.

coveralls · 2017-12-17T01:32:47Z

Coverage increased (+0.05%) to 80.527% when pulling 1e1f699 on vii:master into 65eb529 on libevent:master.

…oming connections. This is important, as otherwise clients can easily exhaust the file descriptors available on a libevent HTTP server, which can cause problems in other code which does not handle EMFILE well: for example, see bitcoin/bitcoin#11368

vii · 2017-12-17T17:49:24Z

as far as I can tell, all the HTTP regression tests fail in that CI mode so the new one failing too is to be expected

Would you like any more changes @azat ?

azat · 2017-12-17T17:56:21Z

as far as I can tell, all the HTTP regression tests fail in that CI mode so the new one failing too is to be expected

Well most of the time tests passes.
But there are flacky tests that we should address.

Would you like any more changes @azat ?

Nope, I will take a look through this again today, thanks for fixing all notes!

coveralls · 2017-12-17T23:04:41Z

Coverage increased (+0.03%) to 80.51% when pulling 40ea136 on vii:master into 6e5c15d on libevent:master.

coveralls · 2017-12-17T23:04:41Z

Coverage increased (+0.03%) to 80.51% when pulling 40ea136 on vii:master into 6e5c15d on libevent:master.

azat · 2017-12-18T00:03:50Z

Applied, thanks! (but one small comment, please take a look at the differences between upstream commit and your patch, since I had been fixed small coding style issues)

vii · 2017-12-18T03:47:05Z

appreciate the help with the coding style @azat! BTW, here is the other small change, that was omitted in the final version of this #581

This is a refined version of the logic previously in #578 The rationale is that the consumer of sockets may wish to temporarily delay accepting for some reason (e.g. being out of file-descriptors). The kernel will then queue them up. The kernel queue is bounded and programs like NodeJS will actually try to quickly accept and then close (as the current behaviour before this PR). However, it seems that libevent should allow the user to choose whether to accept and respond correctly if the listener is disabled.

vii mentioned this pull request Dec 1, 2017

Raise the open fd limit to the maximum allowed bitcoin/bitcoin#11785

Closed

azat requested changes Dec 11, 2017

View reviewed changes

vii force-pushed the master branch 2 times, most recently from 6e1a7e1 to 1e1f699 Compare December 16, 2017 17:48

azat reviewed Dec 16, 2017

View reviewed changes

vii force-pushed the master branch from 1e1f699 to 40ea136 Compare December 17, 2017 17:45

azat closed this in 727bcea Dec 18, 2017

vii mentioned this pull request Dec 18, 2017

Immediately stop trying to accept more connections if listener disabled #581

Closed

fjahr mentioned this pull request May 22, 2020

bitcoin core crashes when too many rpc calls are made bitcoin/bitcoin#11368

Closed

fjahr mentioned this pull request May 22, 2020

Release date for 2.2 #1024

Closed

		case 3:
		/* The length of the method string is 3, meaning it can only be one of two methods: GET or PUT */

HTTP server accept throttle callback #578

HTTP server accept throttle callback #578

Uh oh!

Conversation

vii commented Dec 1, 2017

Uh oh!

coveralls commented Dec 1, 2017

Uh oh!

coveralls commented Dec 1, 2017

Uh oh!

vii commented Dec 4, 2017

Uh oh!

azat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vii Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azat commented Dec 11, 2017

Uh oh!

vii commented Dec 12, 2017

Uh oh!

coveralls commented Dec 16, 2017

Uh oh!

coveralls commented Dec 16, 2017

Uh oh!

azat commented Dec 16, 2017

vii Dec 16, 2017 •

edited

Loading