Search Unity

Question NativeCollection instantiated within a job is write-only by default?

Discussion in 'Entity Component System' started by mannyhams, May 24, 2021.

  1. mannyhams

    mannyhams

    Joined:
    Feb 6, 2015
    Posts:
    34
    InvalidOperationException: The Unity.Collections.NativeHashMap`2[System.ValueTuple`2[System.Int32,System.Int32],System.ValueTuple`2[System.Int32,System.Int32]] has been declared as [WriteOnly] in the job, but you are reading from it.​

    This error arises when I traverse a `NativeHashMap` instantiated within a job. It happens when the `foreach` loop invokes the iterator's `MoveNext` method:

    Code (CSharp):
    1. public bool MoveNext()
    2.             {
    3. #if ENABLE_UNITY_COLLECTIONS_CHECKS
    4.                 AtomicSafetyHandle.CheckReadAndThrow(m_Safety);
    5. #endif
    6.                 return m_Enumerator.MoveNext();
    7.             }
    Unity info:
    Unity: 2021.1.5f1 Personal
    "com.unity.burst": "1.4.8"
    "com.unity.collections": "0.15.0-preview.21"​

    I'm confused because I don't explicitly make the `NativeHashMap` write-only when I instantiate it (nor read-only, nor anything else). I haven't found any guidance in the docs or source code some looking around. Is creating a `NativeCollection` within a job an antipattern?

    This part of my job traverses some map data in flood-fill fashion and creates a list of adjacent map nodes which satisfy a number of conditions.

    Apologies for the complex and unclean prototype code below, I haven't been able to reproduce the problem in a minimal example (iterating a NativeHashMap just works without complaint...). Note that I've removed some amount of irrelevant noise from the code:

    Code (CSharp):
    1. (NativeHashMap<(int x, int z), ConnectedPlot> plotDict, NativeList<(SquareDirection, (int x, int z))> paths)
    2.         GetPlotsWithinDistance(int x, int z, GetPlotsWithinDistanceOptions? options = null)
    3.     {
    4.         options ??= new GetPlotsWithinDistanceOptions();
    5.         var neighbors = new NativeHashMap<(int x, int z), ConnectedPlot>(20, Allocator.Temp);
    6.         var paths = new NativeList<(SquareDirection, (int x, int z))>(20, Allocator.Temp);
    7.         var origin = (x, z);
    8.         var toCheck =
    9.             new NativeHashMap<(int x, int z), (int pathStartIndex, int pathLength)>(1, Allocator.Temp)
    10.             {
    11.                 {origin, (0, 0)}
    12.             };
    13.  
    14.         for (var distance = 1; distance <= options.MaxDistanceLimitInclusive; distance++)
    15.         {
    16.             var nextToCheck =
    17.                 new NativeHashMap<(int x, int z), (int pathStartIndex, int pathLength)>(20, Allocator.Temp);
    18.             int offsetDistance = options.CalcDistance ? distance + options.DistanceOffset : 0;
    19.  
    20.             //
    21.             //  ERROR YIELDED ON NEXT LINE:
    22.             // InvalidOperationException: The Unity.Collections.NativeHashMap`2[System.ValueTuple`2[System.Int32,System.Int32],System.ValueTuple`2[System.Int32,System.Int32]] has been declared as [WriteOnly] in the job, but you are reading from it.
    23.             //
    24.             foreach (KeyValue<(int x, int z), (int pathStartIndex, int pathLength)> pair in toCheck)
    25.             {
    26.                 (int pathStartIndex, int pathLength) pathKey = pair.Value;
    27.                 var path =
    28.                     new NativeSlice<(SquareDirection direction, (int x, int z) coords)>(paths, pathKey.pathStartIndex, pathKey.pathLength);
    29.  
    30.                 foreach (SquareDirection direction in SquareDirectionExtensions.AllDirections())
    31.                 {
    32.                     MaybeAddAndEnqueuePlotAtDirection(direction);
    33.                 }
    34.  
    35.  
    36.                 void MaybeAddAndEnqueuePlotAtDirection(SquareDirection direction)
    37.                 {
    38.                     // Code which conditionally populates neighbors, paths, and nextToCheck
    39.                 }
    40.             }
    41.  
    42.             toCheck.Clear();
    43.             foreach (KeyValue<(int x, int z), (int pathStartIndex, int pathLength)> keyValue in nextToCheck)
    44.             {
    45.                 toCheck.Add(keyValue.Key, keyValue.Value);
    46.             }
    47.  
    48.             nextToCheck.Dispose();
    49.         }
    50.  
    51.         toCheck.Dispose();
    52.  
    53.         return (neighbors, paths);
    54.     }
    TL;DR: Idk why my NativeHashMap "toCheck" is becoming write-only, such that its iterator's `MoveNext` method would complain when it's used in a `foreach` loop.
     
  2. mannyhams

    mannyhams

    Joined:
    Feb 6, 2015
    Posts:
    34
    Ah, this appears to be a known issue (see bottom of the linked page).

    IIUC, all containers instantiated in the same thread which use Allocator.Temp share a single resource under the covers (AtomicSafetyHandle) such that, unbeknownst to the programmer, you can perform an operation on one of the collections which invalidates the enumerators on all the other collections.

    Suggestion for the devs:
    1. An error message specific to this situation would be useful here - it's not clear to the programmer (at least to me) when you attempt to iterate over an invalidated enumerator. Maybe a new AtomicSafetyErrorType? Just stating that the collection is write-only now leads to head-scratching.
    2. It's also unclear to me when enumerators get invalidated, I didn't see that mentioned in the docs but maybe I missed it. Also the concept of an enumerator becoming invalid is sort of foreign to me, though I'm no whiz with C# and I guess it can make sense when we're talking about fixed blocks of native memory.
     
  3. Deleted User

    Deleted User

    Guest

    Faced the same issue, replacing the foreach loops by for(int i = 0; i < list.length; ++i) fixed it but it doesnt apply well to HashSets.
     
    mannyhams likes this.
  4. tonytopper

    tonytopper

    Joined:
    Jun 25, 2018
    Posts:
    226
    I am wondering if I am having a similar problem with a NativeSlice. Same write-only error. I am slicing a NativeList though; don't know how that complicates things.

    The solution for me was copying the values from the NativeSlice into a NativeArray and then reading from that.

    Doesn't seem graceful, but got me passed the error for now.
     
  5. SGStino

    SGStino

    Joined:
    Jul 8, 2014
    Posts:
    10
    This defect will be addressed in a future release.

    I feel like it's still there, I have 4 lists that i want to allocate in a job, but as soon as i use Temp allocator, it throws random errors about being disposed or write-only..

    My workaround has been to use TempJob as allocator.

    But I believed that the TempJob Allocator wasn't allowed to be used from inside a Job, but for me this seems to work.
     
  6. razamgar

    razamgar

    Joined:
    Apr 7, 2020
    Posts:
    14
    Hit with roughly the same problem. Have a NativeList previously allocated using
    Allocator.Temp
    , now allocated a NativeParallelHashMap using the same allocation type, foreaching over NativeList, all that stuff works, but when I add an entry to NativeParallelHashMap, all hell breaks loose - an InvalidOperationException gets thrown, complaining that the NativeList was declared as
    [WriteOnly]
    . Replacing foreach with a for loop helps, as @Liquid_Nalee found out as well, but why? Wish someone from Unity like, say @elliotc-unity, took a look at this.

    Good find. Seems like that's been an issue for at least two years, judging by the date the documentation page has been generated. Heck, this thread itself has had its second birthday two days ago.
     
    Last edited: May 26, 2023
    Mxill likes this.
  7. n3b

    n3b

    Joined:
    Nov 16, 2014
    Posts:
    56
    That's because foreach triggers this:
    Code (CSharp):
    1.         public Enumerator GetEnumerator()
    2.         {
    3. #if ENABLE_UNITY_COLLECTIONS_CHECKS
    4.             AtomicSafetyHandle.CheckGetSecondaryDataPointerAndThrow(m_Safety);
    5.             var ash = m_Safety;
    6.             AtomicSafetyHandle.UseSecondaryVersion(ref ash);
    7. #endif
    There is little to no reason to use Native collections over Unsafe collections when instantiated within the job. There will be no concurrent access, hence the safety handles are useless.
     
    razamgar likes this.