Thrift
  1. Thrift
  2. THRIFT-465

ThreadFactoryTests.h increments iterator after erasing the element to which it refers

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.1
    • Fix Version/s: 0.2
    • Component/s: Test Suite
    • Labels:
      None
    • Environment:

      Mac OS X 10.5.6

    • Patch Info:
      Patch Available

      Description

      In lib/cpp/src/consurrency/test/ThreadFactoryTests.h the reapNThreads() function has this loop:
      for (std::set<shared_ptr<Thread> >::const_iterator thread = threads.begin(); thread != threads.end(); thread++)

      { threads.erase(*thread); }

      but erasing the element from the container invalidates all iteratots that point to that element, so this may result in undefined behavior.

      I believe the equivalent is this:
      while (!threads.empty())

      { threads.erase (*(threads.begin())); }

      although it really seems like you could just call threads.clear() as well and forgo the loop. But maybe there's some subtlety here that I'm missing.

        Activity

        Hide
        Rush Manbert added a comment -

        Patch file.

        Show
        Rush Manbert added a comment - Patch file.
        Hide
        Marc Kwiatkowski added a comment -

        I think clear is ok here too.

        Show
        Marc Kwiatkowski added a comment - I think clear is ok here too.
        Hide
        Rush Manbert added a comment -

        Should I submit a different patch that calls clear?

        Show
        Rush Manbert added a comment - Should I submit a different patch that calls clear?
        Hide
        David Reiss added a comment -

        Can we just delete this entire block since the set's destructor will delete all of the elements?

        Show
        David Reiss added a comment - Can we just delete this entire block since the set's destructor will delete all of the elements?
        Hide
        Rush Manbert added a comment -

        That works too. But I see that you resolved it already. What did you end up doing?

        Show
        Rush Manbert added a comment - That works too. But I see that you resolved it already. What did you end up doing?

          People

          • Assignee:
            Unassigned
            Reporter:
            Rush Manbert
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development