Message Validators #55

vyzo · 2018-01-13T10:24:44Z

Implements asynchronous message validators.
Continuation of #45 which was merged and rebased in a feature branch so that I can continue work on it.
Closes #46.
Closes #56.

cc @whyrusleeping @keks

used for range variable inside goroutine, now passed as argument

- make validators time out after 100ms - add context param to validator functions - add type Validator func(context.Context, *Message) bool - drop message if more than 10 messages are already being validated

vyzo · 2018-01-13T12:37:43Z

Removed the global validation throttle and cleaned up the subscription validation logic.
Let's reopen review?

reword pushMsg for less indentation nesting.

whyrusleeping · 2018-01-13T18:05:47Z

floodsub.go

+		return
+	}
+
+	go func() {


hrm... spawning a goroutine for each message being sent is a bit wonky. Maybe:

select { case p.sendMsg <- .....: default: go func() { p.sendMsg <- .... }() }

Good point, we only have to spawn a new goroutine if it's a publish, in which case the push happens within the event loop.
I'll update.

we are always in the event loop actually. I added a buffer to sendMsg so that we almost always avoid the goroutine.

vyzo · 2018-01-13T18:04:52Z

floodsub.go

@@ -54,6 +58,9 @@ type PubSub struct {
 	// topics tracks which topics each of our peers are subscribed to
 	topics map[string]map[peer.ID]struct{}

+	// sendMsg handles messages that have been validated
+	sendMsg chan sendReq


I am thinking that this should be a chan *sendReq for consistency with the other channels.

vyzo · 2018-01-13T18:09:09Z

floodsub.go

 // NewFloodSub returns a new FloodSub management object
-func NewFloodSub(ctx context.Context, h host.Host) *PubSub {
+func NewFloodSub(ctx context.Context, h host.Host, opts ...Option) (*PubSub, error) {


I think we could avoid the interface change if we don't use any options -- do we want this for future proofing?

Let's keep it, it's a nice interface; also we may want to pass an option for global validation throttle.

and defaultMaxConcurrency is defaultValidateConcurrency.

whyrusleeping · 2018-01-13T19:20:46Z

subscription.go

+	vctx, cancel := context.WithTimeout(ctx, sub.validateTimeout)
+	defer cancel()
+
+	go func() {


annoying that we have to use an extra goroutine for this, but probably okay.

yeah, that's how it go es :)

Can we not just trust that validate will respect its context?

Stebalien

Nice!

Stebalien · 2018-01-13T19:31:58Z

subscription.go

+	vctx, cancel := context.WithTimeout(ctx, sub.validateTimeout)
+	defer cancel()
+
+	go func() {


Can we not just trust that validate will respect its context?

Stebalien · 2018-01-13T19:35:27Z

floodsub.go

+	select {
+	case p.sendMsg <- sreq:
+	default:
+		go func() {


I'm not a fan of this pattern (too easy to open a bunch of go routines). Could we just call maybePublishMessage directly and rename the sendMsg channel name to validatedMsgs or something?

I added a buffer to sendMsg so that we should almost never overflow.
But you are right, we can just call maybePublishMessage!
I'll keep the sendMsg channel named as it is, because it indicates a message that is ready to send.

I skipped the dance, and called maybePublishMessage directly .

Stebalien · 2018-01-13T19:38:49Z

floodsub.go

+				<-p.validateThrottle
+			}()
+		default:
+			log.Warningf("message validation throttled; dropping message from %s", src)


We'll probably want per-peer or per-topic throttles in the future but, for now, this is probably fine.

we already have those! That was part of the improvements in this PR (although most of the stuff got rewritten).

I see... I'm worried that it's still possible to starve a topic by maxing out the global validator first. Ideally, we'd take the per-topic ticket first, then the global one.

I think it would be hard to do, because a throttled subscription would release the semaphore quickly.

I improved the comments around the throttle to more clearly describe the intention and existence of per-topic throttles.

Stebalien · 2018-01-13T19:42:42Z

floodsub.go

+			continue
+		}
+
+		rch := make(chan bool, 1)


I'd just use a single channel and count results from it. Allocating channels isn't that expensive but it's certainly not free.

fair enough; it was really convenient to write this way though :)

alright, implemented!

…idation

vyzo · 2018-01-13T23:50:35Z

so f1be0f1, which removed the external goroutine enforcing the timeout bombs two of keks' tests -- they explicitly test the external goroutine timing out.

vyzo · 2018-01-14T00:10:41Z

Removed the two offending tests, as they test faulty behaviour.

More to the point, per discussion with @Stebalien on irc: if we timeout externally, the faulty validator is still running, while we unthrottle and leak a goroutine. It is preferable to have the faulty validator to cause the topic to throttle, at which point it can be identified as a bug and be debuged, rather than having a silent goroutine leak.

vyzo · 2018-01-17T08:24:05Z

@Stebalien ping -- let's move this forward?

Stebalien · 2018-01-18T03:00:43Z

I don't see how this PR is a step in the right direction.

Validators should either be a property of the topic (registered at initialization, preferably) or validators should be a property of the subscription in which case a message's validity for subscription A shouldn't affect the message's validity for subscription B. Given that making validators a property of the subscription makes it difficult to determine if we should forward the message, validators should be a property of the topic.

vyzo · 2018-01-18T07:40:22Z

Ok, I will rewrite to implement per topic validators then.

vyzo · 2018-01-18T17:19:47Z

I have removed the per-subscription validators and implemented per-topic validations.
Only a single validator per topic is allowed, registered with RegisterTopicValidator; supporting multiple validators per topic would unnecessarily complicate the api and implementation and didn't seem worth it for now.

vyzo · 2018-01-18T17:28:29Z

summoning @Stebalien @whyrusleeping

whyrusleeping · 2018-01-18T18:07:48Z

floodsub.go

+// pushMsg pushes a message performing validation as necessary
+func (p *PubSub) pushMsg(vals []*topicVal, src peer.ID, msg *Message) {
+	if len(vals) > 0 {
+		// validation is asynchronous and globally throttled with the throttleValidate semaphore.


Is this comment still true?

Yes it is still true.
I can remove the global throttle if we deem so, I just like to have a limit on how much goncurrency we can spawn as a result of incoming network traffic.

although, subscription reference below! that's not true, I will fix the comment.

whyrusleeping

My current understanding is that we still have global validators, but no global throttling right? I still see a pubsub level validate throttle though, so theres still some global throttling?

whyrusleeping · 2018-01-18T18:15:47Z

Oh wait, I was thinking backwards, no global validators, but still global throttling.

vyzo · 2018-01-18T18:20:00Z

My current understanding is that we still have global validators, but no global throttling right? I still see a pubsub level validate throttle though, so theres still some global throttling?
Oh wait, I was thinking backwards, no global validators, but still global throttling.

Right, we have a global throttle that applies to all validators, that's set to a reasonably high limit (8192 active goroutines). This is to protect us from blowing our resource usage from big bursts of traffic that trigger validations.

Each individual topic validator has its own throttle, that limits the resource usage per topic. This throttle by default is set much lower, to a mere 100 active goroutines.

As I said we can remove the global throttle if we deem so, and we can also tweak the defaults accordingly.

Stebalien

I'd be fine accepting this if @whyrusleeping is as I consider it a step in the right direction this time but I'm still not happy with it (see #58). Why are you so opposed to having validators decide which topics they care about? The way this currently works, subscribing to the same topic twice will be painful.

vyzo · 2018-01-19T07:59:40Z

Why are you so opposed to having validators decide which topics they care about?

Hrm, I am not terminally opposed to them -- I just find the interface a little clunky.

The way this currently works, subscribing to the same topic twice will be painful.

Why? The validator registration is decoupled from the subscription and it's something we do at initialization.

jvsteiner · 2018-01-20T20:25:49Z

Hey, I'm a brand-newbie in looking at this project, but for me, in the use cases I have in mind, I'd like to be able to validate by topic (although I can see why "by subscription" would also be useful) - but perhaps more importantly, I'd like not to propagate messages to other peers, when they do not validate. I might even want to drop peers who send invalid messages. I guess peer attribution might be a bit outside the scope of what's currently possible - but that's my $0.02.

vyzo · 2018-01-20T20:31:30Z

@jvsteiner with this pr, we do exactly that: we don't broadcast messages that don't validate on a per-topic basis. We have rejected subscription-based validators because of incoherent semantics.

The global validators will simply add an additional layer below per topic validators: they will allow us to define validator classes for pubsub namespaces and will be implemented in follow up work.

Stebalien · 2018-01-24T00:29:23Z

@whyrusleeping I believe we're waiting on you for the final signoff.

keks and others added 11 commits January 13, 2018 12:12

optionally allow caller to validate messages

647bb98

typedef subscription options and fix typo

930f264

log when validator discards message

1945f89

vet

7dd4e0b

used for range variable inside goroutine, now passed as argument

ungxify

197a598

better tests for validation

89e6a06

complete validator functions

02877cd

- make validators time out after 100ms - add context param to validator functions - add type Validator func(context.Context, *Message) bool - drop message if more than 10 messages are already being validated

fix timeout

6e8b9f2

make validator timeout configurable

fe09d1e

make maximum concurrency configurable, split loop

88274db

fix dangling maxConcurrency reference

4241241

ghost assigned vyzo Jan 13, 2018

ghost added the in progress label Jan 13, 2018

vyzo mentioned this pull request Jan 13, 2018

Allow caller to optionally validate messages #45

Closed

vyzo added 2 commits January 13, 2018 12:33

WithValidator and WithValidatorTimeout are subscription options

d2f6a00

per subscription validation throttle and more efficient dispatch logic

982c4de

vyzo requested review from whyrusleeping, keks and Stebalien January 13, 2018 12:37

vyzo added 2 commits January 13, 2018 14:44

code cosmetics

fba445b

reword pushMsg for less indentation nesting.

add validation context for cancelation on aborts

c95ed28

whyrusleeping reviewed Jan 13, 2018

View reviewed changes

vyzo commented Jan 13, 2018

View reviewed changes

vyzo added 2 commits January 13, 2018 20:11

don't always spawn a goroutine for sending a new message

5ef13c7

the sendMsg channel should yield pointers for consistency

bf2151b

vyzo force-pushed the feat/validators branch from 04146f5 to bf2151b Compare January 13, 2018 18:53

vyzo added 2 commits January 13, 2018 20:56

WithMaxConcurrency is WithValidatorConcurrency

856a25c

and defaultMaxConcurrency is defaultValidateConcurrency.

install global validation throttle, use reasonable defaults.

edcb251

whyrusleeping reviewed Jan 13, 2018

View reviewed changes

sendMsg should have a buffer

473a5d2

Stebalien requested changes Jan 13, 2018

View reviewed changes

vyzo added 3 commits January 13, 2018 21:56

pushMsg should just call maybePublishMessage when it doesn't need val…

f6081fb

…idation

use a single channel for all validation results

145a84a

don't spawn an extra goroutine for the validator context

f1be0f1

Stebalien mentioned this pull request Jan 13, 2018

Per topic validation #56

Closed

remove faulty tests

cb365a5

improved comment about global validation throttle

fceb00d

implement per topic validators

bbdec3f

whyrusleeping reviewed Jan 18, 2018

View reviewed changes

fix comment, subscriptions don't have validators any more.

3f4fc21

whyrusleeping reviewed Jan 18, 2018

View reviewed changes

Stebalien mentioned this pull request Jan 18, 2018

Global Validators #58

Open

Stebalien approved these changes Jan 18, 2018

View reviewed changes

Stebalien merged commit c82e67d into master Jan 24, 2018

Stebalien deleted the feat/validators branch January 24, 2018 06:03

ghost removed the in progress label Jan 24, 2018

Message Validators #55

Message Validators #55

Conversation

vyzo commented Jan 13, 2018 • edited Loading

vyzo commented Jan 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo Jan 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Jan 13, 2018

vyzo commented Jan 14, 2018

vyzo commented Jan 17, 2018

Stebalien commented Jan 18, 2018

vyzo commented Jan 18, 2018

vyzo commented Jan 18, 2018 • edited Loading

vyzo commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping left a comment

Choose a reason for hiding this comment

whyrusleeping commented Jan 18, 2018

vyzo commented Jan 18, 2018

Stebalien left a comment

Choose a reason for hiding this comment

vyzo commented Jan 19, 2018

jvsteiner commented Jan 20, 2018 • edited Loading

vyzo commented Jan 20, 2018

Stebalien commented Jan 24, 2018

vyzo commented Jan 13, 2018 •

edited

Loading

vyzo Jan 13, 2018 •

edited

Loading

vyzo Jan 13, 2018 •

edited

Loading

vyzo commented Jan 18, 2018 •

edited

Loading

jvsteiner commented Jan 20, 2018 •

edited

Loading