Remove flowMonitor key after the MultipartReply is received #64
Conversation
ofctrl/ofSwitch.go
Outdated
| replyChan <- rep | ||
| switch t.Type { | ||
| case openflow15.MultipartType_FlowDesc: | ||
| if self.monitorEnabled { |
There was a problem hiding this comment.
In my opinion, the following code would be a bit safer:
key := fmt.Sprintf("%d", t.Xid)
ch, found := monitoredFlows.Get(key)
if found {
if self.monitorEnabled {
replyChan := ch.(chan *openflow15.MultipartReply)
replyChan <- rep
}
monitoredFlows.Remove(key)
}this way we make sure that the key is removed from the map if present, regardless of the value of self.monitorEnabled. I prefer it because the rest of the code doesn't really offer strong guarantees, such as DumpFlowStats only being called when self.monitorEnabled is true.
There was a problem hiding this comment.
the latest version is not what I suggested
I was suggesting checking for existence of the key even if self.monitorEnabled is false, to ensure proper cleanup (also, ideally, entries in the map that are not removed should be expired after some time, in case no reply is ever received?).
ofctrl/ofSwitch.go
Outdated
| replyChan <- rep | ||
| switch t.Type { | ||
| case openflow15.MultipartType_FlowDesc: | ||
| if self.monitorEnabled { |
There was a problem hiding this comment.
the latest version is not what I suggested
I was suggesting checking for existence of the key even if self.monitorEnabled is false, to ensure proper cleanup (also, ideally, entries in the map that are not removed should be expired after some time, in case no reply is ever received?).
After "monitorEnabled" is enabled, an OpenFlow FlowDesc message is sent to dump the flow stats if a user calls OFSwitch.DumpFlowStats or Flow.MonitorRealizeStatus. At this time the message's transactionsID is added into a concurrent map monitoredFlows. The issue is the transactionID is not removed after the reply of FlowDesc message is received. This change is to fix the issue. Signed-off-by: wenyingd <wenyingd@vmware.com>
After "monitorEnabled" is enabled, an OpenFlow FlowDesc message is sent to dump the flow stats if a user calls
OFSwitch.DumpFlowStatsorFlow.MonitorRealizeStatus. At this time the message's transactionsID is added into a concurrent mapmonitoredFlows. The issue is the transactionID is not removed after the reply of FlowDesc message is received.This change is to fix the issue.
Fix: #63