Avro
  1. Avro
  2. AVRO-1092

avro-c: improving thread safety in error management code

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.6.3, 1.7.0
    • Fix Version/s: 1.7.0
    • Component/s: c
    • Labels:
      None

      Description

      Error management code isn`t thread safe at all. I wrote a patch for this issue, but it works only for *nix systems.

      Affected functions: avro_set_error(), avro_prefix_error()

      1. AVRO-1092.patch
        5 kB
        Pugachev Maxim
      2. AVRO-1092-patch-2.patch
        1 kB
        Vivek Nadkarni

        Activity

        Hide
        Douglas Creager added a comment -

        Oh and I described how to set the THREADSAFE cmake variable in the INSTALL file.

        Show
        Douglas Creager added a comment - Oh and I described how to set the THREADSAFE cmake variable in the INSTALL file.
        Hide
        Douglas Creager added a comment -

        Committed to SVN with Vivek's Windows-compatibility updates, and without setting -pthread compiler flag manually.

        Show
        Douglas Creager added a comment - Committed to SVN with Vivek's Windows-compatibility updates, and without setting -pthread compiler flag manually.
        Hide
        Douglas Creager added a comment -

        Looks good except for one detail — we don't have to add -pthread to the compiler flags since FindThreads will add that for us if it's needed on the current platform.

        Show
        Douglas Creager added a comment - Looks good except for one detail — we don't have to add -pthread to the compiler flags since FindThreads will add that for us if it's needed on the current platform.
        Hide
        Vivek Nadkarni added a comment -

        Thanks for the clarification.

        Cheers,
        Vivek

        Show
        Vivek Nadkarni added a comment - Thanks for the clarification. Cheers, Vivek
        Hide
        Pugachev Maxim added a comment -

        > 1. Are THREAD_LIBRARIES and THREADSAFE cmake intrinsic definitions or are they your definitions?

        This is mine definitions.

        > 2. I didn't see why the definition _REENTRANT was set. It isn't used anywhere in the source. Is it a requirement of pthreads?

        Defining _REENTRANT causes the compiler to use thread safe (i.e. re-entrant) versions of several functions in the C library. This is not a requirement, but it should be used in MT code.

        > 3. How do you disable or enable threads (under Linux)?

        You need to pass a THREADSAFE flag to cmake: "cmake -DTHREADSAFE=true ."

        > Is there a reason you didn't use the syntax similar to the zlib and lzma codecs?

        There is no flag named "Threads_FOUND" in cmake`s module FindThreads.cmake. In perfect scenario this code should be like this:

        if(THREADSAFE)
        find_package(Threads)
        add_definitions(-DTHREADSAFE -D_REENTRANT)
        set(THREADS_LIBRARIES $

        {CMAKE_THREAD_LIBS_INIT})

        #check for Unix/Win32 and add extra definitions
        endif(THREADSAFE)

        ${CMAKE_THREAD_LIBS_INIT}

        contains correct thread library (pthreads or win32 threads).

        Show
        Pugachev Maxim added a comment - > 1. Are THREAD_LIBRARIES and THREADSAFE cmake intrinsic definitions or are they your definitions? This is mine definitions. > 2. I didn't see why the definition _REENTRANT was set. It isn't used anywhere in the source. Is it a requirement of pthreads? Defining _REENTRANT causes the compiler to use thread safe (i.e. re-entrant) versions of several functions in the C library. This is not a requirement, but it should be used in MT code. > 3. How do you disable or enable threads (under Linux)? You need to pass a THREADSAFE flag to cmake: "cmake -DTHREADSAFE=true ." > Is there a reason you didn't use the syntax similar to the zlib and lzma codecs? There is no flag named "Threads_FOUND" in cmake`s module FindThreads.cmake. In perfect scenario this code should be like this: if(THREADSAFE) find_package(Threads) add_definitions(-DTHREADSAFE -D_REENTRANT) set(THREADS_LIBRARIES $ {CMAKE_THREAD_LIBS_INIT}) #check for Unix/Win32 and add extra definitions endif(THREADSAFE) ${CMAKE_THREAD_LIBS_INIT} contains correct thread library (pthreads or win32 threads).
        Hide
        Vivek Nadkarni added a comment -

        A few questions regarding the patch, specifically the CMakeLists.txt
        file. I am by no means a CMake expert, so apologies if the answers
        should be obvious .

        1. Are THREAD_LIBRARIES and THREADSAFE cmake intrinsic definitions or
        are they your definitions?

        2. I didn't see why the definition _REENTRANT was set. It isn't used
        anywhere in the source. Is it a requirement of pthreads?

        3. How do you disable or enable threads (under Linux)? Is there a
        reason you didn't use the syntax similar to the zlib and lzma
        codecs? For example

        find_package(Threads)
        if (Threads_FOUND)
        message("Threads_FOUND")

        1. Use threads here.
          else (Threads_FOUND)
          message("Threads not FOUND")
          endif(Threads_FOUND)

        Thanks,
        Vivek

        Show
        Vivek Nadkarni added a comment - A few questions regarding the patch, specifically the CMakeLists.txt file. I am by no means a CMake expert, so apologies if the answers should be obvious . 1. Are THREAD_LIBRARIES and THREADSAFE cmake intrinsic definitions or are they your definitions? 2. I didn't see why the definition _REENTRANT was set. It isn't used anywhere in the source. Is it a requirement of pthreads? 3. How do you disable or enable threads (under Linux)? Is there a reason you didn't use the syntax similar to the zlib and lzma codecs? For example find_package(Threads) if (Threads_FOUND) message("Threads_FOUND") Use threads here. else (Threads_FOUND) message("Threads not FOUND") endif(Threads_FOUND) Thanks, Vivek
        Hide
        Vivek Nadkarni added a comment -

        I downloaded the patch to test it out in Windows, since you had called
        out the potential Windows incompatibility. The code didn't compile
        under Windows, but I was able to get the code to compile and pass
        tests with a minor modification.

        I am attaching my minor modification as a patch that should be applied
        on top of AVRO-1092.patch. This patch should be applied from within
        the avro-trunk/lang/c directory.

        My patch also updates the file README.maintaining_win32.txt, to
        capture the inability of MSVC++ to support structure initialization
        using element names.

        Show
        Vivek Nadkarni added a comment - I downloaded the patch to test it out in Windows, since you had called out the potential Windows incompatibility. The code didn't compile under Windows, but I was able to get the code to compile and pass tests with a minor modification. I am attaching my minor modification as a patch that should be applied on top of AVRO-1092 .patch. This patch should be applied from within the avro-trunk/lang/c directory. My patch also updates the file README.maintaining_win32.txt, to capture the inability of MSVC++ to support structure initialization using element names.
        Hide
        Pugachev Maxim added a comment -

        Patch for this issue.

        Show
        Pugachev Maxim added a comment - Patch for this issue.

          People

          • Assignee:
            Unassigned
            Reporter:
            Pugachev Maxim
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development