Timeout #38

JoaoAndreSa · 2020-03-10T13:00:09Z

Closes #30

I added the timeouts for all channels. I've added a default 10min value that can be easily changed at runtime. Is this enough?

cgrigis

Looks good to me!

Minor note: the break for no-op branches in select/switch is not necessary (see e.g. https://play.golang.org/p/3kj1htOkqNK), but it does not hurt. :)

ineiti

Some questions

ineiti · 2020-03-11T06:00:06Z

Makefile

@@ -27,7 +27,7 @@ test_lint:
 	}

 test_local:
-	go test -v -race -short -p=1 ./...
+	go test -v -short -p=1 ./...


That's a dangerous change! You should be more concerned to change the tests so that they don't time-out. The -race saved us on numerous occasions, so if you decide to really go without it, I'd make sure that the other members of the team are OK with it.

So I removed the race because while I was updating onet I started having this error for the simulations that only disappears after removing the -race flag. I'm not sure where is this coming from :/ Trying to debug what is the problem so I can re-add the flag.

The plot thickens... apparently this only happens locally

I think that is bbolt's error in conjunction with the latest go. Are you already using go 1.14?

@tharvik - what is the state of bbolt and the latest go?

yes. If it's the case I can go back to a previous version (which I did and it works)

@ineiti bbolt is still broken, we need etcd-io/bbolt#201 before updating onet/cothority (but containerd and kubernetes both waits on that, it will be fixed in a matter of days)

ineiti · 2020-03-11T06:01:36Z

protocols/shuffling_protocol.go

@@ -116,6 +118,10 @@ func NewShufflingProtocol(n *onet.TreeNodeInstance) (onet.ProtocolInstance, erro
 			break
 		}
 	}
+
+	// default timeout
+	dsp.Timeout = 10 * time.Minute


How has this timeout been chosen?

Why are there different timeouts?

Shouldn't it be possible to change the timeout, e.g., with an environment variable?

There was no rule to chose the timeout. I just chose enough time for it not to fail in normal query scenarios. As for the timeouts I added one for each different protocol or service. This way we can pass it when calling the protocol or service. You think an environment variable would be better?

It depends (as always):

do you trust the leader to not send a malicious timeout that will keep your resources? (I suppose yes)

do you want to have each node decide on his own timeouts? (I suppose no)

So with a yes - no answer, I think you're doing the right thing ;)

But instead of hardcoding the timeout, I'd use https://golang.org/pkg/os/#Getenv to read the MEDCO_TIMEOUT environment variable, and, if it is not set, use a globally defined timeout.

Timeouts are always hard to get right (even @bford said "good luck" when I had to do it ;), so having them easily adjustable also from the outside is a plus!

@ineiti I followed your suggestion and changed the timeouts 3dcc5b4
Would this be better?

yes, looks good to me.

JoaoAndreSa · 2020-03-11T09:24:46Z

Looks good to me!

Minor note: the break for no-op branches in select/switch is not necessary (see e.g. https://play.golang.org/p/3kj1htOkqNK), but it does not hurt. :)

Removed break

ineiti · 2020-03-11T12:44:44Z

protocols/collective_aggregation_protocol.go

@@ -201,14 +197,19 @@ func (p *CollectiveAggregationProtocol) Dispatch() error {

 // Announce forwarding down the tree.
 func (p *CollectiveAggregationProtocol) aggregationAnnouncementPhase() error {
+	timeout, err := time.ParseDuration(os.Getenv("MEDCO_TIMEOUT"))


It should be enough to put this in afunc init() at the beginning of the file constants.go. Then you can remove it from all the methods you're calling it...

done abbe277

ineiti · 2020-03-11T13:32:34Z

lib/constants.go

+func init() {
+	tmp, err := time.ParseDuration(os.Getenv("MEDCO_TIMEOUT"))
+	if err != nil {
+		TIMEOUT = tmp


Last comment:

if len(os.Getenv("MEDCO_TIMEOUT") > 0 { log.Warn("Couldn't parse MEDCO_TIMEOUT") }

This will save you hours later on ;)

I've added it in 5564657
However now that I updated to the new onet version 3.1.1 the error with the "E : overlay.go:613 (v3.(*Overlay).nodeDelete) - Error while closing node: Can't shutdown empty ProtocolInstance:" pops way more often (TestServiceClearAttr). Do you know what changed with this new 3.1 version of Onet?

It disappears if I remove the error condition when calling tn.SetConfig()

Do you have a simple test-case that I can run to trigger it?

And what do you mean by "removing the error condition when calling tn.SetConfig()"?

@ineiti see here #38 (comment)

^TestServiceClearAttr$ in github.com/ldsec/unlynx/services

I mean this

tn.SetConfig(conf)

instead of

if err := tn.SetConfig(conf); err != nil { return nil, xerrors.Errorf("couldn't set config: %+v", err) }

fixed #38 (comment)

tharvik · 2020-03-11T14:23:36Z

protocols/collective_aggregation_protocol.go

-			return errors.New("Error sending <DataReferenceMessage>:" + err.Error())
+	select {
+	case dataReferenceMessage := <-p.DataReferenceChannel:
+		if !p.IsLeaf() {


Unneded if, SendToChildren when having none should work.

Arf, I misexplained it; the if !p.IsLeaf() is unnecessary, as a Leaf sending to it's children is a NOP.
And you're droping the error here.

ahh I did not get it :P
Ok it's fixed: f3812c3

tharvik · 2020-03-11T14:25:35Z

protocols/collective_aggregation_protocol.go

+	case dataReferenceMessage := <-p.DataReferenceChannel:
+		if !p.IsLeaf() {
+			if err := p.SendToChildren(&dataReferenceMessage.DataReferenceMessage); err != nil {
+				return errors.New("Error sending <DataReferenceMessage>:" + err.Error())


You can use the new fmt.Errorf when returning error (it will also keep the stacktrace).

Suggested change

return errors.New("Error sending <DataReferenceMessage>:" + err.Error())

return fmt.Errorf("Error sending <DataReferenceMessage>: %v", err)

switched everywhere to fmt.Errorf: 8861342

tharvik · 2020-03-11T14:27:25Z

protocols/shuffling+ddt_protocol.go

+	select {
+	case tmp = <-p.PreviousNodeInPathChannel:
+	case <-time.After(libunlynx.TIMEOUT):
+		return errors.New(p.ServerIdentity().String() + " didn't get the <tmp> on time.")


Maybe a better name than tmp?

I tried to clean a bit these tmp variables. I changed this one to spDDTbs (referring to the type of the object shufflingPlusDDTBytesStruct) even though IMHO is not much better :P

a4d36eb

tharvik · 2020-03-11T14:29:49Z

services/service.go

-	if err != nil {
-		return nil, err
-	}
+	tn.SetConfig(conf)


Don't drop the error.

If I don't drop the error 7f37465 some tests stop working
e.g. - ^TestServiceClearAttr$ in github.com/ldsec/unlynx/services

- couldn't set config: Can't set config twice: go.dedis.ch/onet/v3.(*TreeNodeInstance).SetConfig /Users/jagomes/Go/pkg/mod/go.dedis.ch/onet/v3@v3.1.1/treenode.go:879:

That might indicate a bug :P

Service.NewProtocol is handled specially by onet, to start new protocol on the children of the root node of the tree. To fix it, move the SetConfig in StartProtocol, just before calling NewProtocol, this way, only root will actually set the config for the whole tree, ensuring that every node get's it before starting.

Ok this is perfect it works like a charm 6340084

That might indicate a bug :P

Service.NewProtocol is handled specially by onet, to start new protocol on the children of the root node of the tree. To fix it, move the SetConfig in StartProtocol, just before calling NewProtocol, this way, only root will actually set the config for the whole tree, ensuring that every node get's it before starting.

wow - @tharvik gets an onet expert! I know whom I'll ask for my next bug ;)

tharvik · 2020-03-11T14:33:42Z

simul/proofs_verification_simul.go

+
+			log.Lvl1(len(results), " proofs verified")
+
+			if results[0] == false {


Suggested change

if results[0] == false {

if !results[0] {

changed this everywhere e5509ba

mickmis · 2020-03-12T11:33:30Z

I agree with @ineiti comment about having it configurable as environment variable. We can that way set it easily in MedCo through the Docker environment.
It should also be documented somewhere, like in the README.md.

mickmis

See:

Looks OK otherwise (modulo other's comments), thanks!

JoaoAndreSa · 2020-03-12T12:32:46Z

Shouldn't be called DEFAULT_TIMEOUT rather than TIMEOUT ?

So I can't use DEFAULT_TIMEOUT because it goes against golang naming conventions so I kept the same name.

Lint errors:
lib/constants.go:34:5: don't use ALL_CAPS in Go names; use CamelCase

8563eee

JoaoAndreSa · 2020-03-12T12:33:54Z

I agree with @ineiti comment about having it configurable as environment variable. We can that way set it easily in MedCo through the Docker environment.
It should also be documented somewhere, like in the README.md.

It's already done. There is this function to set it up based on the environment variable MEDCO_TIMEOUT

func init() {
	tmp, err := time.ParseDuration(os.Getenv("MEDCO_TIMEOUT"))
	if err == nil {
		DEFAULT_TIMEOUT = tmp
	} else {
		log.Warn("Couldn't parse MEDCO_TIMEOUT, using default value: ", DEFAULT_TIMEOUT.String())
	}
}

…go lint error)

ineiti · 2020-03-12T16:47:38Z

app/client.go

@@ -98,7 +98,7 @@ func parseQuery(el *onet.Roster, sum string, count bool, where, predicate, group
 	whereRegex := "{(w[0-9]+(,\\s*[0-9]+))*(,\\s*w[0-9]+(,\\s*[0-9]+))*}"
 	groupByRegex := "{g[0-9]+(,\\s*g[0-9]+)*}"

-	if !checkRegex(sum, sumRegex) {
+	if checkRegex(sum, sumRegex) == false {


? I thought @tharvik 's suggestion was to change from == false to !, no? Anyway, that would be my preference.

you're right I didn't see it correctly :(

fixed e5509ba

ineiti · 2020-03-12T16:48:26Z

Reality check: CoVid-19, people stay at home, no coffee talk, and everybody gathers around @JoaoAndreSa PR to tell him how to code... Sorry guy!

JoaoAndreSa · 2020-03-16T17:15:35Z

I think I addressed all the reviews/suggestions. :) Let me know (@ineiti @tharvik @mickmis @cgrigis) if I forgot something.

JoaoAndreSa added 3 commits March 10, 2020 11:23

Added select timeouts for unlynx protocols

ec63e48

Added select timeouts for unlynx protocols/utils

b0d3547

Added select timeouts for unlynx services and simulations

66925ca

JoaoAndreSa requested review from mickmis and cgrigis March 10, 2020 13:00

Updated libraries except Onet

17479ba

cgrigis approved these changes Mar 10, 2020

View reviewed changes

ineiti suggested changes Mar 11, 2020

View reviewed changes

JoaoAndreSa added 3 commits March 11, 2020 10:25

Removed break and re-adding race flag

2c8b8bd

Re-added simulation tests

3281921

Changed timeouts to use os.Getenv

3dcc5b4

JoaoAndreSa requested a review from ineiti March 11, 2020 11:38

ineiti suggested changes Mar 11, 2020

View reviewed changes

Added init() for timeout

abbe277

ineiti reviewed Mar 11, 2020

View reviewed changes

JoaoAndreSa added 3 commits March 11, 2020 14:33

Fixed init() in constants.go mistake in condition

3a7f339

Added warning while parsing timeout

5564657

Removed error condition around tn.SetConfig

3031cda

tharvik reviewed Mar 11, 2020

View reviewed changes

JoaoAndreSa added 3 commits March 11, 2020 17:00

Removed unecessary if condition when sending empty structs

31c25d5

Switched from errors.new to fmt.Errorf

8861342

Remove the isLeaf condition for the SendToChildren call

f3812c3

mickmis suggested changes Mar 12, 2020

View reviewed changes

Changed TIMEOUT constant to DEFAULT_TIMEOUT

5bc1e5d

JoaoAndreSa added 2 commits March 12, 2020 13:39

Fixed go lint warning

186b6f4

Revert name to TIMEOUT because of golang naming convention (to avoid …

8563eee

…go lint error)

JoaoAndreSa added 5 commits March 12, 2020 14:36

Cleaned some of the tmp variables

a4d36eb

Corrected if negative conditions

c5dddb8

Added error condition of tn.SetConfig for debug

7f37465

Fixed tn.SetConfig bug

6340084

Merge branch 'dev' of https://github.com/ldsec/unlynx into timeout

e9c05b5

ineiti reviewed Mar 12, 2020

View reviewed changes

Reverted if conditions to the correct way

e5509ba

ineiti approved these changes Mar 16, 2020

View reviewed changes

JoaoAndreSa merged commit 6372d0e into dev Mar 18, 2020

JoaoAndreSa deleted the timeout branch March 18, 2020 10:53

	return errors.New("Error sending <DataReferenceMessage>:" + err.Error())
	return fmt.Errorf("Error sending <DataReferenceMessage>: %v", err)


		log.Lvl1(len(results), " proofs verified")

		if results[0] == false {

Timeout #38

Timeout #38

Conversation

JoaoAndreSa commented Mar 10, 2020

cgrigis left a comment

Choose a reason for hiding this comment

ineiti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoaoAndreSa Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoaoAndreSa Mar 11, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoaoAndreSa commented Mar 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ineiti Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

JoaoAndreSa Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoaoAndreSa Mar 12, 2020 • 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

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

JoaoAndreSa Mar 12, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoaoAndreSa Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

mickmis commented Mar 12, 2020

mickmis left a comment

Choose a reason for hiding this comment

JoaoAndreSa commented Mar 12, 2020 • edited Loading

JoaoAndreSa commented Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoaoAndreSa Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

ineiti commented Mar 12, 2020

JoaoAndreSa commented Mar 16, 2020

JoaoAndreSa Mar 11, 2020 •

edited

Loading

JoaoAndreSa Mar 11, 2020 •

edited

Loading

ineiti Mar 11, 2020 •

edited

Loading

JoaoAndreSa Mar 11, 2020 •

edited

Loading

JoaoAndreSa Mar 12, 2020 •

edited

Loading

JoaoAndreSa Mar 12, 2020 •

edited

Loading

JoaoAndreSa Mar 12, 2020 •

edited

Loading

JoaoAndreSa commented Mar 12, 2020 •

edited

Loading

JoaoAndreSa commented Mar 12, 2020 •

edited

Loading

JoaoAndreSa Mar 16, 2020 •

edited

Loading