-
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
Allow custom timeouts in RPCLock #95
Conversation
Goober, test and merge |
@@ -24,7 +24,7 @@ func RenewContract(w Wallet, tpool TransactionPool, id types.FileContractID, key | |||
} | |||
s.host = host | |||
defer s.Close() | |||
if err := s.Lock(id, key); err != nil { | |||
if err := s.Lock(id, key, 10*time.Second); err != nil { |
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'm not sure if 10 sec. is enough for renewing.
This comment suggests that a host takes time to retrieve a contract if it has many contracts. If it's correct, it sounds like locking the renewing contract here also would take time.
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.
Choosing default durations is hard. I agree that 10 seconds is probably too short. The maximum is 10 minutes. Maybe 1 or 2 minutes? You can always use (Session).RenewContract
if you need a custom timeout.
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.
1 min. might be good. How long is the default timeout in hosts?
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.
Sorry, I meant the maximum allowed by the host is 10 minutes. The RHP itself doesn't say anything about the default or maximum timeout.
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.
Interesting. If I set the timeout to 5 min., we get an "unexpected EOF" error while if I set the timeout to 1 min., we get timeout error instead. I hence thought host's timeout is less than 5 min.
Then, the "unexpected EOF" error means the host closes the connection without sending a correct response before the timeout, and some exceptions should happen in the host.
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.
oh man. I think this is actually a bad bug in the host. It looks like the host sets a timeout of 2 minutes for the Lock RPC...regardless of the timeout requested by the renter. 👀
So that definitely explains why longer timeouts are failing for you. 2 minutes still seems like a long time to wait, but I guess when hosts are under load it can happen. We should definitely fix that bug, but may in the meantime we can work around it. I'm thinking that we should switch to 1 minute timeouts, and if we fail to acquire the lock (not because of timeout, but because the host explicitly told us it couldn't get the lock), we just immediately try again.
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.
We've set the timeout to 3 min. and got "Lock: couldn't read LoopLock response: read tcp 10.244.0.136:49564->72.50.192.180:9982: i/o timeout". I think this means the host closed the connection after 2min. but our server still thought the connection was alive.
Supersedes #90