-
Notifications
You must be signed in to change notification settings - Fork 16
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
Added timer to the stream outbound function in case ovs stops responding. #31
base: main
Are you sure you want to change the base?
Conversation
util/stream.go
Outdated
@@ -126,14 +131,17 @@ func (m *MessageStream) outbound() { | |||
} | |||
|
|||
klog.V(4).InfoS("Sent", "dataLength", len(data), "data", len(data), data) | |||
case <-time.After(messageTimeout): |
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.
Question: If we add timeout control in MessageStream outbound functions, do we need other similar control in ofnet?
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.
This may cause the connnection unstable if no message is sent in "messageTimeout"?
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.
Maybe you meant this?
for {
select {
case <-m.Shutdown:
klog.Infof("Closing OpenFlow message stream.")
m.conn.Close()
close(m.parserShutdown)
return
case msg := <-m.Outbound:
// Forward outbound messages to conn
data, _ := msg.MarshalBinary()
writeErrorCh := make(chan error)
go func() {
_, err := m.conn.Write(data)
writeErrorCh<-err
}()
select {
case err := <-writeErrorCh:
if err != nil {
klog.ErrorS(err, "OutboundError")
m.Error <- err
m.Shutdown <- true
} else {
klog.V(4).InfoS("Sent", "dataLength", len(data), "data", len(data), data)
}
case <-time.After(messageTimeout):
m.Error <- errors.New("Write to socket timed out")
m.Shutdown <- true
}
close(writeErrorCh)
}
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.
My bad, put the timer under wrong scope. Would fix this.
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.
To your question:
Question: If we add timeout control in MessageStream outbound functions, do we need other similar control in ofnet?
No we should not have any timer in the ofnet code. ofnet code should either get a success or an error and should not expect libopenflow/stream code to block indefinitely.
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 code looks good to me, I just want to confirm if any existing API can be used in the net.Conn to control timeout in Write action
Having an offline discussion with @ashish-varma, no better solution is found. Would you give some suggestions @tnqn @antoninbas |
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.
LGTM
I had the same question as @wenyingd, and I don't see an answer here yet. Why can't we use |
It make more sense to use SetWriteDeadline. I am testing the code with this and update the commit. |
in case ovs stops responding. Fixed naming of some of the variables to more appropriate names. Signed-off-by: Ashish Varma <ashishvarma.ovs@gmail.com>
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.
LGTM
@antoninbas @tnqn Do you have other comments on this change? |
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.
LGTM
@@ -119,6 +123,7 @@ func (m *MessageStream) outbound() { | |||
case msg := <-m.Outbound: | |||
// Forward outbound messages to conn | |||
data, _ := msg.MarshalBinary() | |||
m.conn.SetWriteDeadline(time.Now().Add(messageTimeout)) |
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.
nit: maybe we should handle the error
return value here?
Fixed naming of some of the variables to more appropriate names.