Question "Do : Loop Until Completed" Feels like a bad idea?

[Flippedbeyond]

    Sub ReloadRoster()
        Dim Completed As Boolean = False

        AddHandler OnRosterEnd, Sub() Completed = True
        AddHandler OnRosterItem, Sub(sender As Object, item As RosterItem)
                                     Roster.Add(item)
                                 End Sub
        RequestRoster()
        Do : Loop Until Completed
    End Sub

I feel like this is a bad idea, what do you guys think? Basically i just want the sub to complete only after the OnRosterEnd event has fired, if this is a bad idea, any suggestions?

 

[JohnH]

It is a bad idea, the loop will consume 100% cpu. The code should probably be arranged around RosterEnd event instead. There don't seem to be a reason for you to stop the method, but if there is it should perhaps be based on signaling instead, see EventWaitHandle, AutoResetEvent, CountdownEvent, and ManualResetEvent
Maybe the RosterEnd handler should raise some kind of Completed event instead that invoker can handle as appropriate.

 

[Flippedbeyond]

Thanks JohnH. I didn't get the chance to do too much reading, but i think for now a AutoResetEvent will do the trick. What do you think of this?

    Sub ReloadRoster()
        Dim ActionCompleted As New Threading.AutoResetEvent(False)

        Dim RosterLoadEnd As ObjectHandler = Sub(sender As Object) ActionCompleted.Set()
        Dim RosterAddItem As RosterHandler = Sub(sender As Object, item As RosterItem)
                                                                              _Roster.Add(item)
                                                                          End Sub
        AddHandler OnRosterEnd, RosterLoadEnd
        AddHandler OnRosterItem, RosterAddItem

        RequestRoster()

        If Not ActionCompleted.WaitOne(2000) Then
            Throw New Exception("Could not load Roster")
        End If

        RemoveHandler OnRosterEnd, RosterLoadEnd
        RemoveHandler OnRosterItem, RosterAddItem
    End Sub

 

[JohnH]

Yes, and no, if you throw exception RemoveHandler won't be called.

 

[Flippedbeyond]

Thanks, good catch. fixed it. Gotta test it but i think this should do it.

    Sub ReloadRoster()
        Dim action As New Threading.AutoResetEvent(False)

        Dim RosterLoadEnd As ObjectHandler = Sub(sender As Object) action.Set()
        Dim RosterAddItem As RosterHandler = Sub(sender As Object, item As RosterItem)
                                                                              _Roster.Add(item)
                                                                          End Sub

        AddHandler OnRosterEnd, RosterLoadEnd
        AddHandler OnRosterItem, RosterAddItem

        RequestRoster()

        Dim actionCompleted As Boolean = action.WaitOne(2000)

        RemoveHandler OnRosterEnd, RosterLoadEnd
        RemoveHandler OnRosterItem, RosterAddItem

        If Not actionCompleted Then
            Throw New Exception("Could not load Roster")
        End If
    End Sub

 

[JohnH]

Also, don't throw instances of the base Exception type. It is also not recommended that consumers handle such non-specific exceptions. Use a more specific type, for example TimeoutException.

 

[Flippedbeyond]

I'll change that, Thanks JohnH.