Skip to content

all edges, IIPs not getting removed with removeNode #142

Closed
forresto opened this Issue Feb 20, 2014 · 3 comments

1 participant

@forresto
NoFlo member

Gotcha: mutating an array while iterating over it.

for edge in @edges
  continue unless edge
  if edge.from.node is node.id
    @removeEdge edge.from.node, edge.from.port
  if edge.to.node is node.id
    @removeEdge edge.to.node, edge.to.port

for initializer in @initializers
  continue unless initializer
  if initializer.to.node is node.id
    @removeInitial initializer.to.node, initializer.to.port

https://github.com/noflo/noflo/blob/master/src/lib/Graph.coffee#L301

... I have seen this a couple of places, we should decide on a better reduce pattern.

@forresto
NoFlo member

How about this?

toRemove = []
for initializer in @initializers
  if initializer.to.node is node.id
    toRemove.push initializer
for initializer in toRemove
  @removeInitial initializer.to.node, initializer.to.port
@forresto
NoFlo member

Bug when there are multiple IIPs to the same port:

  removeInitial: (node, port) ->
    @checkTransactionStart()

    for edge, index in @initializers
      continue unless edge
      if edge.to.node is node and edge.to.port is port
        @emit 'removeInitial', edge
        @initializers.splice index, 1

    @checkTransactionEnd()

fix:

  removeInitial: (node, port) ->
    @checkTransactionStart()

    toRemove = []
    toKeep = []
    for edge, index in @initializers
      if edge.to.node is node and edge.to.port is port
        toRemove.push edge
      else
        toKeep.push edge
    @initializers = toKeep
    for edge in toRemove
      @emit 'removeInitial', edge

    @checkTransactionEnd()

?

@forresto
NoFlo member

Do we use the underloaded form for removeEdge anywhere? If not, this would fix the bug and make things simpler:

    toRemove = []
    toKeep = []
    for edge,index in @edges
      if edge.from.node is node and edge.from.port is port and edge.to.port is node2 and edge.to.port is port2
        toRemove.push edge
      else
        toKeep.push edge
    @edges = toKeep
    for edge in toRemove
      @emit 'removeEdge', edge

edit: found https://github.com/noflo/noflo/blob/master/src/lib/shell.coffee#L103

@forresto forresto closed this in 1025428 Feb 27, 2014
@KevinHoward KevinHoward added a commit to KevinHoward/noflo that referenced this issue Jun 30, 2014
@forresto forresto test that catches array gotcha with removeNode #142 f199070
@KevinHoward KevinHoward added a commit to KevinHoward/noflo that referenced this issue Jun 30, 2014
@forresto forresto mutating an array while iterating over it fix #142 c9bd0bb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.