Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-465

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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 Rush Manbert added a comment -

        Patch file.

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

        I think clear is ok here too.

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

        Should I submit a different patch that calls clear?

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

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

        Show
        dreiss 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 Rush Manbert added a comment -

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

        Show
        rush 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 Rush Manbert
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development