-
Notifications
You must be signed in to change notification settings - Fork 4
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
Timeout #38
Timeout #38
Changes from 4 commits
ec63e48
b0d3547
66925ca
17479ba
2c8b8bd
3281921
3dcc5b4
abbe277
3a7f339
5564657
3031cda
31c25d5
8861342
f3812c3
5bc1e5d
186b6f4
8563eee
a4d36eb
c5dddb8
7f37465
6340084
e9c05b5
e5509ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,23 @@ | ||
module github.com/ldsec/unlynx | ||
|
||
go 1.14 | ||
|
||
require ( | ||
github.com/BurntSushi/toml v0.3.1 | ||
github.com/Knetic/govaluate v3.0.0+incompatible | ||
github.com/cpuguy83/go-md2man/v2 v2.0.0 // indirect | ||
github.com/fanliao/go-concurrentMap v0.0.0-20141114143905-7d2d7a5ea67b | ||
github.com/gorilla/websocket v1.4.1 // indirect | ||
github.com/montanaflynn/stats v0.6.3 // indirect | ||
github.com/r0fls/gostats v0.0.0-20180711082619-e793b1fda35c | ||
github.com/satori/go.uuid v1.2.0 | ||
github.com/stretchr/testify v1.3.0 | ||
github.com/urfave/cli v1.22.1 | ||
go.dedis.ch/kyber/v3 v3.0.5 | ||
github.com/stretchr/testify v1.4.0 | ||
github.com/urfave/cli v1.22.3 | ||
go.dedis.ch/kyber/v3 v3.0.12 | ||
go.dedis.ch/onet/v3 v3.0.24 | ||
golang.org/x/crypto v0.0.0-20200302210943-78000ba7a073 // indirect | ||
golang.org/x/sys v0.0.0-20200302150141-5c8b2ff67527 // indirect | ||
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 // indirect | ||
) | ||
|
||
//replace go.dedis.ch/onet/v3 => ../../../go.dedis.ch/onet |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,6 +10,7 @@ package protocolsunlynx | |||||
import ( | ||||||
"errors" | ||||||
"sync" | ||||||
"time" | ||||||
|
||||||
"github.com/ldsec/unlynx/lib" | ||||||
"github.com/ldsec/unlynx/lib/aggregation" | ||||||
|
@@ -106,6 +107,8 @@ type CollectiveAggregationProtocol struct { | |||||
Proofs bool | ||||||
ProofFunc proofCollectiveAggregationFunction // proof function for when we want to do something different with the proofs (e.g. insert in the blockchain) | ||||||
MapPIs map[string]onet.ProtocolInstance // protocol instances to be able to call protocols inside protocols (e.g. proof_collection_protocol) | ||||||
|
||||||
Timeout time.Duration | ||||||
} | ||||||
|
||||||
// NewCollectiveAggregationProtocol initializes the protocol instance. | ||||||
|
@@ -129,6 +132,9 @@ func NewCollectiveAggregationProtocol(n *onet.TreeNodeInstance) (onet.ProtocolIn | |||||
return nil, errors.New("couldn't register data reference channel: " + err.Error()) | ||||||
} | ||||||
|
||||||
// default timeout | ||||||
pap.Timeout = 10 * time.Minute | ||||||
|
||||||
return pap, nil | ||||||
} | ||||||
|
||||||
|
@@ -195,11 +201,15 @@ func (p *CollectiveAggregationProtocol) Dispatch() error { | |||||
|
||||||
// Announce forwarding down the tree. | ||||||
func (p *CollectiveAggregationProtocol) aggregationAnnouncementPhase() error { | ||||||
dataReferenceMessage := <-p.DataReferenceChannel | ||||||
if !p.IsLeaf() { | ||||||
if err := p.SendToChildren(&dataReferenceMessage.DataReferenceMessage); err != nil { | ||||||
return errors.New("Error sending <DataReferenceMessage>:" + err.Error()) | ||||||
select { | ||||||
case dataReferenceMessage := <-p.DataReferenceChannel: | ||||||
if !p.IsLeaf() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unneded if, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Arf, I misexplained it; the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ahh I did not get it :P |
||||||
if err := p.SendToChildren(&dataReferenceMessage.DataReferenceMessage); err != nil { | ||||||
return errors.New("Error sending <DataReferenceMessage>:" + err.Error()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use the new
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. switched everywhere to fmt.Errorf: 8861342 |
||||||
} | ||||||
} | ||||||
case <-time.After(p.Timeout): | ||||||
return errors.New(p.ServerIdentity().String() + "didn't get the <dataReferenceMessage> on time.") | ||||||
} | ||||||
return nil | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,6 +91,8 @@ type ShufflingProtocol struct { | |
CollectiveKey kyber.Point | ||
ExecTimeStart time.Duration | ||
ExecTime time.Duration | ||
|
||
Timeout time.Duration | ||
} | ||
|
||
// NewShufflingProtocol constructs neff shuffle protocol instances. | ||
|
@@ -116,6 +118,10 @@ func NewShufflingProtocol(n *onet.TreeNodeInstance) (onet.ProtocolInstance, erro | |
break | ||
} | ||
} | ||
|
||
// default timeout | ||
dsp.Timeout = 10 * time.Minute | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It depends (as always):
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 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, looks good to me. |
||
|
||
return dsp, nil | ||
} | ||
|
||
|
@@ -186,9 +192,22 @@ func (p *ShufflingProtocol) Start() error { | |
func (p *ShufflingProtocol) Dispatch() error { | ||
defer p.Done() | ||
|
||
shufflingBytesMessageLength := <-p.LengthNodeChannel | ||
var shufflingBytesMessageLength shufflingBytesLengthStruct | ||
select { | ||
case shufflingBytesMessageLength = <-p.LengthNodeChannel: | ||
break | ||
case <-time.After(p.Timeout): | ||
return errors.New(p.ServerIdentity().String() + "didn't get the <shufflingBytesMessageLength> on time.") | ||
} | ||
|
||
var tmp shufflingBytesStruct | ||
select { | ||
case tmp = <-p.PreviousNodeInPathChannel: | ||
break | ||
case <-time.After(p.Timeout): | ||
return errors.New(p.ServerIdentity().String() + "didn't get the <tmp> on time.") | ||
} | ||
|
||
tmp := <-p.PreviousNodeInPathChannel | ||
sm := ShufflingMessage{} | ||
if err := sm.FromBytes(tmp.Data, shufflingBytesMessageLength.CVLengths); err != nil { | ||
return err | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plot thickens... apparently this only happens locally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. If it's the case I can go back to a previous version (which I did and it works)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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)