Reference type function argument does not retain changes after function try catch block

Question

Code below.

I completely understand reference vs value types. [correction, I do not completely understand reference vs value types.]

Today I ran into something odd.
I declare a datatable class var and initialize it as null.
I create function: GetAutoAllocationResult(in int, datatable, out string)
I pass pass the datatable class var to the function.
I make some conditional changes inside a TRY/CATCH block.
I return from the function.
The datatable does not retain changes from within the function.
If I use the OUT keyword in the function signature for the datatable, it retains changes.
What?

DataTable auto_allocation_result = null;

// ...


// get the data from the stored procedure return
string auto_allocation_query_result_message = string.Empty;
bool is_auto_allocation_result_query_successful = false;
is_auto_allocation_result_query_successful = GetAutoAllocationResult(auto_allocation_record_uid, 
                        auto_allocation_result, out auto_allocation_query_result_message);

//...


private bool GetAutoAllocationResult(in int p_auto_allocate_record_uid, DataTable p_return_data, string p_message)
{
    bool is_successful = false;
    p_return_data = new DataTable();
    p_message = string.Empty;

    string sql = some select statement

    SqlCommand sc = new SqlCommand();
    sc.Connection = some sql connection
    sc.CommandText = sql;


    // add parameters
    SqlParameter sqlparam_order_ship_loc_uid = sc.Parameters.Add("@order_ship_loc_uid", SqlDbType.Int);
    sqlparam_order_ship_loc_uid.Value = p_auto_allocate_record_uid;
    


    // run query
    try
    {
        SqlDataAdapter sda = new SqlDataAdapter(sc);
        sda.Fill(p_return_data);   // p_return_data DOES NOT RETAIN DATA AFTER FUNCTION CALL IF OUT KEYWORD IS NOT USED

        if (p_return_data != null)
        {
            if (p_return_data.Rows.Count <= 0)
            {
                p_return_data = null;

                p_message =
                    $"Could not perform auto-allocation.  Please provide IT with the information below.{Environment.NewLine}" +
                    $"{Environment.NewLine}" +
                    $"No auto allocation rows returned for UID ({p_auto_allocate_record_uid}).";

                is_successful = false;
            }
            else
            {
                is_successful = true;
            }
        }
        else
        {
            p_message =
                $"Could not perform auto-allocation.  Please provide IT with the information below.{Environment.NewLine}" +
                $"{Environment.NewLine}" +
                $"Auto allocation query did not instantiate a datatable ({p_auto_allocate_record_uid}).";

            is_successful = false;
        }
    }
    catch (Exception ex)
    {
        p_message =
            $"Failed to perform auto-allocation.  Please provide IT with the information below.{Environment.NewLine}" +
            $"{Environment.NewLine}" +
            $"There was an error querying auto allocation result data.{Environment.NewLine}" +
            $"Exception Message: ({ex.Message}){Environment.NewLine}" +
            $"Inner Exception: ({ex.InnerException}){Environment.NewLine}" +
            $"Stack Trace: ({ex.StackTrace})";

        is_successful = false;
    }


    return is_successful;            
}

*"I completely understand reference vs value types."* - Let's revisit that assertion when you're done here. It's not entirely clear that you do. — madreflection, May 24 '22 at 22:59
`p_return_data = new DataTable();` -- The caller never sees this instance without `out`. Remove that line and use the instance you're given (check for `null` first!). — madreflection, May 24 '22 at 23:03
@madreflection - So, a datatable is a reference type. There is no need to create a function signature using a REF/OUT keyword for a reference type parameter when you want it to retain changes. Why is this different? — ILoveSQL, May 24 '22 at 23:04
See my second comment. You're assigning a reference to a new instance to that variable. The caller never sees that instance. Without `out`, the caller still has the a reference to the instance that it passed. — madreflection, May 24 '22 at 23:05
It's different because you're making the changes to an instance that's discarded at the end of the method. But it's not a difference in reference types, it's a difference in what you're doing with them. Again, remove that assignment. — madreflection, May 24 '22 at 23:09
Same with `p_message`. You can assign it, but you're just assigning a new instance to that variable. And since strings are immutable, there's nothing you can change on that instance, so the caller will never see any changes to it. — madreflection, May 24 '22 at 23:14
As an aside, you don't need `in` on `int` parameters. Use it on "large" structs to avoid wholesale copies of the contents. On a 64-bit system, `in` on an `int` actually doubles how much is passed because it has to pass a 64-bit reference instead of a 32-bit integer. — madreflection, May 24 '22 at 23:16
in keyword assures any changes to the argument do not persist outside the function. out on p_message assures that it retains its changes after the function call, which is what I want. — ILoveSQL, May 24 '22 at 23:20
It does, but on a value type, you don't need it in order to ensure that. It does that by not allowing you to assign anything to it in the first place, so there's nothing to retain. It's the same as `ref` except that it's also read-only. — madreflection, May 24 '22 at 23:21
I definitely over/incorrectly used IN there. You are absolutely correct. A value type will not retain changes unless passed by ref/out. Thank you for that observation. — ILoveSQL, May 24 '22 at 23:35
To put it another way, your method can change the properties of the object referred to by the caller's reference variable. But without `out`, it cannot change which object the caller's variable refers to. — Kevin Krumwiede, May 24 '22 at 23:42

Stewart · Accepted Answer · 2022-05-24T23:25:18.467

0

You need to set it to the new DataTable rather than null outside the function. Remember a reference is a pointer to a memory object. But the reference itself is passed by value (on the stack/copied). So you've passed in a reference that doesn't point to a memory object (null) and the copy has been assigned to the DataTable inside the function. When the function finishes the copy is removed from the stack/disgarded and the DataTable goes to the garbage collector.

It's an easy mistake to make if you're not experienced. But now you should have a better understanding for your future coding fun.

edited May 24 '22 at 23:25

answered May 24 '22 at 23:19

Stewart

705
3
6

1

You need to remove the new DataTable() assignment inside the function. That's again pointing it to a new object in memory. – Stewart May 24 '22 at 23:27
New call (and function adjustment) does not change the outcome: ` //get the data from the stored procedure return string auto_allocation_query_result_message = string.Empty; bool is_auto_allocation_result_query_successful = false; auto_allocation_result = new DataTable(); is_auto_allocation_result_query_successful = GetAutoAllocationResult(auto_allocation_record_uid, auto_allocation_result, out auto_allocation_query_result_message); ` – ILoveSQL May 24 '22 at 23:27
I told you about that in my second comment above. – madreflection May 24 '22 at 23:27
I learned a couple of things today: I understand reference types vs value types only so far. so my question is: if you have a reference type like a datatable that is passed in, and the intention is to populate it with some data, how would you correctly scrub an incoming argument like this without setting it to a new datatable? – ILoveSQL May 24 '22 at 23:47
Don't scrub it. You have to ask yourself, "Do I really need it to be empty, or is it okay to add to the provided `DataTable`?" Your caller (which you can't assume is also you) may not appreciate that you cleared the data table. If you must return only the data that the method is responsible for retrieving, use `out` and create your own `DataTable` instance. – madreflection May 24 '22 at 23:56
@ILoveSQL what development environment are you using? FYI I use Visual Studio 2022, on a massive 100 project web app, and VS has a very handy hot rebuild icon/command. So for small changes you can do a rebuild without having to stop your app running. – Stewart May 24 '22 at 23:57
Or, arguably better, `return` the `DataTable` instance on success, and throw an exception (instead of the output message parameter) on failure. – madreflection May 24 '22 at 23:58
It is our ERP, where, developing extensions require the cycling of app pools to ingest an updated DLL (new DLLs are ingested fine). – ILoveSQL May 25 '22 at 00:53
Is there anything wrong with this approach? Inside the function, create a new datatable and set the incoming datatable argument = to the new datatable? This assumes the current setup (not using OUT and initializing the datatable prior to the function call) – ILoveSQL May 25 '22 at 00:55
1

Thanks for the green tick. If you feel you need to create a new DataTable inside the function, the best approach is to return the DataTable from the function, rather than bool, and not pass it in at all. If unsuccessful return null, otherwise return the DataTable. And for clarity rename your function to GetAutoAllocationResultDataTable. – Stewart May 25 '22 at 01:11
1

Give it a name that aligns with what you're actually returning, like `GetOrderShipLocation`. I can't suggest a better name because all you gave us to go on was the parameter name. – madreflection May 25 '22 at 01:16
1

In the future, you would do well to create a [mre] of the problem. Write a console application that has a similar function, which just returns dummy data, and is called from `Main`. That will allow you to figure out how parameter passing works without dealing with the details and the environment of the ERP system, and make changes rapidly in response to suggestions. – madreflection May 25 '22 at 01:20
Is this why the question was marked as not meeting the stack overflow guidelines? For sake of argument; I stated it was reproducible by toggling an OUT keyword. I posted source code from top to bottom, so what exactly did I miss besides my opening statement being invalid? – ILoveSQL May 25 '22 at 13:48
For my part, I voted to close as "caused by a typo" because I considered the `p_return_data = new DataTable();` assignment to be a simple oversight, probably written early in the development process and overlooked due to testing without the debugger, and all you had to do was remove it. That close reason would result in the message you see, but so do others. – madreflection May 25 '22 at 17:00
Being reproducible is one thing, but not being *minimal* (as was the case here) meant that you were bogged down by the ceremony of deploying the extension to the ERP system in order to test even the simplest of change. Long turnaround leads to resistance to try things, which then leads to pushback and unnecessary discussion. The problem had nothing to do with the environment in which it ran. It could have been demonstrated, tested, and resolved within a simple console application. That involves much less friction so it can be done in a much tighter feedback loop. – madreflection May 25 '22 at 17:00
Once you've solved the problem in an isolated context, you could apply what you learned to the code for the extension. I suggest you review [How to create a Minimal, Reproducible Example](https://stackoverflow.com/help/minimal-reproducible-example). – madreflection May 25 '22 at 17:01

Reference type function argument does not retain changes after function try catch block

1 Answers1