CodeSOD collection

Zecc

@BernieTheBernie said in CodeSOD collection:

It gets harder when you do lazy initialization in a multithreaded context.

FTFY.

topspin

@BernieTheBernie said in CodeSOD collection:

comes Johnny and "improves" it:

At least that one is easily explained to him as the double checked locking pattern has a name.

Can't you reduce the time spent holding the lock by doing the following, though (depends on what the elided code does and if it needs the lock):

     private IReadOnlyList<Tuple<int, int>> GetSupportedTemperatureRanges()
     {
         if (m_SupportedTemperatureRanges == null)
         {
             // lots of initialization code
             // ....
             lock (m_EbusParameters.ParamsLocker)
             {
                 if (m_SupportedTemperatureRanges == null)
                 {
                     m_SupportedTemperatureRanges = listTempRanges;
                 }
             }
         }
         return m_SupportedTemperatureRanges;
     }

BernieTheBernie

@topspin Unfortunately, exactly that block requires thread-safe execution. Some stubborn hardware accessed via low-quality third party sdk...
On the other hand, locks are not often required, so executing that lenghty thing in the block does not matter so much.

PleegWat

@topspin said in CodeSOD collection:

Can't you reduce the time spent holding the lock by doing the following, though (depends on what the elided code does and if it needs the lock):

That's redundant, if the lock is only used for that single block. The first thread there still has to do all the work, the second has to wait for it either way. Waiting properly instead of replicating the work can only be faster, and definitely leaves more resources for other processes/threads.

dkf

@PleegWat said in CodeSOD collection:

That's redundant, if the lock is only used for that single block. The first thread there still has to do all the work, the second has to wait for it either way. Waiting properly instead of replicating the work can only be faster, and definitely leaves more resources for other processes/threads.

Double checked locking isn't guaranteed to be correct unless you guarantee that assignment to the field being initialised is atomic and done exactly once, at the end of the code inside the innermost block. (That can be surprisingly difficult to get absolutely right, especially to prove that it is right.) If access is common enough for this to be a problem, use thread-specific variables (all practical languages have those if they've got threads at all) that are initialised from a common piece of code that insists on using proper locking; the TSV acts as a nice lock-free cache of the value pulled from the common locked allocator.

Usually you can instead guarantee it by allocating values during object initialisation. Every object is only ever initialised once after all, and you control when it's handed between threads. (Or if you don't, That's Your Problem…)

PleegWat

@dkf I meant, specifically, pre-calculating the initialised value before taking the lock. Double checking definitely makes sense. You can also use a dedicated primitive like pthread_once().

dkf

@PleegWat pthread_once() is better as that can have the correct memory barriers and other protective stuff in there to deal with the critical atomic update requirements.

Zecc

Seriously now. I've posted this link above: lazy initialization.

Anything wrong with just using Lazy<T>?

BernieTheBernie

Index was outside the bounds of the array. A common error message, isn't it?
The culprit is in this function by Johnny:

private int GetMappingBufferIndexFromValue(IValueWithUnit _value)
{
    int intValue = (int)UnitConversions.Convert(_value, m_UnitMappingBuffer);
    return intValue - m_MappingBufferLowestIntegerValue;
}

It maps some input value to an index of an array. As you can see, there is no check that the value returned is actually a valid index, even the array proper is not mentioned in the code (but its name gets mentioned in the function name).

Off-by-one errors are among the most common errors in software development. So we do not need to care about them.
And it is extremely hard to find them with unit tests. So we do not need to bother with that dreadful task of writing tests.
Thus Johnny makes sure he will be our greatest producer of off-by-one errors.

Then Kevin was hit by the error. He fixed it:

private void UpdateMappingBufferForScale(IScale _scale)
{
    int lowerIndex = GetMappingBufferIndexFromValue(_scale.Range.LowerValue);
    int upperIndex = GetMappingBufferIndexFromValue(_scale.Range.UpperValue);
   // added by Kevin
    if (upperIndex >= m_TempColorMappingBuffer.Length-1)
    {
        upperIndex = m_TempColorMappingBuffer.Length-1;
    }
    // ... some more code
}

Yeah! The bug is fixed in one place (out of several places) in one direction now.

Why not fix the function Bernie mentioned first?

Zecc

I think you meant:

The bug is fixed

BernieTheBernie

@Zecc said in CodeSOD collection:

I think you meant:

The bug is fixed

Or rather the entomological style:
https://alumni.berkeley.edu/sites/default/files/EssigCraneFly.jpg
That bug is now fixed into the code base and won't be able to leave...

BernieTheBernie

Why should someone use an in-built function / property like

CultureInfo.CurrentCulture.DateTimeFormat

when

DateTimeFormatInfo dateTimeInfo = DateTimeFormatInfo.GetInstance(CultureInfo.CurrentCulture);

is possible, too?

Kevin wants to really make sure he gets an actual DateTimeFormatInfo.

But that's not all. Let's look at the context:

protected string DateFormat
{
    get
    {
        DateTimeFormatInfo dateTimeInfo = DateTimeFormatInfo.GetInstance(CultureInfo.CurrentCulture);
        return dateTimeInfo.DateSeparator == "/" ? "MM/dd/yy" : "dd.MM.yy";
    }
}

Kamil Podlesak

@BernieTheBernie said in CodeSOD collection:

Why should someone use an in-built function / property like
CultureInfo.CurrentCulture.DateTimeFormat
when
DateTimeFormatInfo dateTimeInfo = DateTimeFormatInfo.GetInstance(CultureInfo.CurrentCulture);
is possible, too?

Kevin wants to really make sure he gets an actual DateTimeFormatInfo.

But that's not all. Let's look at the context:
protected string DateFormat
{
    get
    {
        DateTimeFormatInfo dateTimeInfo = DateTimeFormatInfo.GetInstance(CultureInfo.CurrentCulture);
        return dateTimeInfo.DateSeparator == "/" ? "MM/dd/yy" : "dd.MM.yy";
    }
}

So, basically

dkf

@Kamil-Podlesak said in CodeSOD collection:

So, basically

And many other places besides.

dcon

@Kamil-Podlesak said in CodeSOD collection:

So, basically

: I don't see the problem...

Kamil Podlesak

@dcon said in CodeSOD collection:

@Kamil-Podlesak said in CodeSOD collection:

So, basically

: I don't see the problem...

It's not a problem, it's a challenge!

BernieTheBernie

@dcon said in CodeSOD collection:

@Kamil-Podlesak said in CodeSOD collection:

So, basically

: I don't see the problem...

Just like Kevin: "there's the US, and then there's the rest of the world who all do things exactly the same correct way we do them."

dkf

@BernieTheBernie said in CodeSOD collection:

Just like Kevin: "there's the US, and then there's the rest of the world who all do things exactly the same correct way we do them."

He should review the order used by …

BernieTheBernie

The biggest WTF is WCF. I've told you that before, haven't I? And let me repeat that with just fresh experiences:

I had a class ClusterHazardLocations which inherited some levels down from IEnumerable<IHazardLocation>. In case of an alarm, an AlarmInfo object is transferred from a sensor device to some other computer showing the information to the user. That AlarmInfo has an IHazardLocations property, and in this case it was a ClusterHazardLocations object. It used to worked.

But then... Then I added a simple function to that ClusterHazardLocations class: public void Add(IHazardLocation other).
Do you already see the problem?
No? Neither did I.
The mere existence of the function - even with an empty body - causes the WCF communication to fail.
Can you guess why?
I do not understand that at all. After all, IEnumerable does not have an Addmethod.

And what was the solution?
I renamed the Addmethod to AddHazardLocation.
Now, it worx again.
Time wasted: some 5 hours.

Thank you, Microsoft!

Kamil Podlesak

@BernieTheBernie said in CodeSOD collection:

The biggest WTF is WCF. I've told you that before, haven't I? And let me repeat that with just fresh experiences:

I had a class ClusterHazardLocations which inherited some levels down from IEnumerable<IHazardLocation>. In case of an alarm, an AlarmInfo object is transferred from a sensor device to some other computer showing the information to the user. That AlarmInfo has an IHazardLocations property, and in this case it was a ClusterHazardLocations object. It used to worked.

But then... Then I added a simple function to that ClusterHazardLocations class: public void Add(IHazardLocation other).
Do you already see the problem?
No? Neither did I.
The mere existence of the function - even with an empty body - causes the WCF communication to fail.
Can you guess why?
I do not understand that at all. After all, IEnumerable does not have an Addmethod.

My money is on reflection-based pythonesque duct-taping: existence of Add method means it's a full-blown collection (and modifiable one to boot).

BernieTheBernie

@Kamil-Podlesak said in CodeSOD collection:

reflection-based pythonesque duct-taping

I fear that. Somehow the Add method is generated by WCF with

[Serializable]
public class ClusterHazardLocations : IClusterHazardLocations
{
    [NotNull]
    private readonly List<IClusterHazardLocation> m_Locations;

In the end, I should rename the class to WcfClusterFuckLocations.

BernieTheBernie

Jason changed the IEnumerableExtensions class. Bernie took a look and found a gem:

public static int IndexOf<T>(this IEnumerable<T> _enumeration, T _elementToFind)
{
    int i = 0;
    foreach (T element in _enumeration)
    {
        if (Equals(element, _elementToFind))
            return i;
        i++;
    }
    return -1;
}

What's wrong with that? Well, IEnumerable has no indexer which could be accessed, in contrast to a List (which implements IEnumerable, but adds extra functionality). So, what can you do with the Index found by Jason's method?

Parody

@BernieTheBernie said in CodeSOD collection:

What's wrong with that? Well, IEnumerable has no indexer which could be accessed, in contrast to a List (which implements IEnumerable, but adds extra functionality). So, what can you do with the Index found by Jason's method?

Count there yourself (via IEnumerable.GetEnumerator and IEnumerator.MoveNext) or use LINQ's .ElementAt extension to do the same thing in slightly prettier code (if it can't index directly).

Ideally, though, the index is used by a caller who does have a collection that can be indexed directly and this is just generic because it doesn't have to be limited to those types of collections.

Zecc

@Parody I'd call .ToList() or .ToArray() on the IEnumerable so that I could then index it without having to re-enumerate it.

But it really depends on whether it's worth the extra allocation. It's hard to know without knowing what's behind the IEnumerable. It could be anything. It could be that .ElementAt(i) is overriden in the classes in use and provides direct indexing behind the curtains. /shrug

In any case, we also don't know what the index returned by the extension is used for. Maybe it's used for something else entirely, like setting the selected item on a dropdown or something. It's not necessarily a bad extension.

Parody

@Zecc said in CodeSOD collection:

It could be that .ElementAt(i) is overriden in the classes in use and provides direct indexing behind the curtains. /shrug

There's a built-in override for IList<>s, IIRC.

In any case, we also don't know what the index returned by the extension is used for. Maybe it's used for something else entirely, like setting the selected item on a dropdown or something. It's not necessarily a bad extension.

True, there may be better ways for code that uses it to get the same result but that's outside the extension's scope. I don't think there's anything wrong with this particular extension on its own.

Tsaukpaetra

Status: I'm having faith that the debugger isn't reporting the state of this variable correctly. I have faith that the needful is being done, and that I'm just imagining the inconsistency. I have faith that when it enters the function call, all will be as expected...

BernieTheBernie

Working with new libraries can sometimes lead to odd results. So I added the 'LanguageExt' library to my projects, in order to add some 'functional' functionality. Looked fine. But...
What's wrong with:

Option<IOrchestra> Orchestra;
...
IOrchestra result = Orchestra.Match(
    Some: o => _Repository.UpdateOrchestra(o, Name, _Homepage, _Info),
    None:  _Repository.AddOrchestra(Name, _Homepage, _Info));

It compiles. No exceptions at runtime. But instead of the Updatemethod, the Addmethod gets called.

Eventually I saw some missing item and corrected it:

IOrchestra result = Orchestra.Match(
    Some: o => _Repository.UpdateOrchestra(o, Name, _Homepage, _Info),
    None: () => _Repository.AddOrchestra(Name, _Homepage, _Info));

But I do not understand the library well enough yet to explain the difference in the beahvior.

bobjanova

@BernieTheBernie In the first snippet you're calling the method, rather than passing a lambda to call it in some scenario. So of course it gets called. I would actually then expect both methods to get called if there is a match. It's weird that the library allows that though (unless AddOrchestra returns a function itself).

Tsaukpaetra

@Tsaukpaetra said in CodeSOD collection:

Status: I'm having faith that the debugger isn't reporting the state of this variable correctly. I have faith that the needful is being done, and that I'm just imagining the inconsistency. I have faith that when it enters the function call, all will be as expected...

It was. So, hooray.

Now to ponder what the proper thing to do to update (or is it copy?) the values from a temporary one of those things to a pointer of that thing as part of a upsert operation in the array the BlobCache is. I'm thinking operator overload?

Applied Mediocrity

@Tsaukpaetra said in CodeSOD collection:

It was

How?

Tsaukpaetra

@Applied-Mediocrity said in CodeSOD collection:

@Tsaukpaetra said in CodeSOD collection:

It was

How?

Fuckin' machic!

topspin

This post is deleted!

Applied Mediocrity

@Tsaukpaetra No, seriously. How? The machic debugger stops all threads and reads memory. I believe I've seen this happen in a really nasty race condition. Otherwise the address it's got is wrong and the entire session is so utterly fucked it cannot be trusted about anything.

Tsaukpaetra

@Applied-Mediocrity I blame UE4 somehow fucking up how things work.

I'm willing to bet some "optimization" is happening behind the scenes and the code I'm breakpointing isn't actually the code that's running.

Zecc

@BernieTheBernie said in CodeSOD collection:

But I do not understand the library well enough yet to explain the difference in the beahvior.

This really doesn't have anything to do with the library in particular. It's just how C# works, or how any other language would for that matter.

Zecc

Anonymized, but otherwise untouched.

    string input1 = input;
    string input2 = null;
    if (input1 != "" || input1 != string.Empty)
    {

        input2 = input1.Remove(input1.LastIndexOf(""));
        entity.Property = input2;
    }
    else
    {
        entity.Property = "";
        input2 = "";
    }

Oh, and input1 is never used again. input2 id used once 60 lines below, to set the Property of another entity.

Zecc

input1.Remove(input1.LastIndexOf(""));

I've confirmed: this will remove the last character of input1, throwing a ArgumentOutOfRangeException if empty.

BernieTheBernie

@Zecc said in CodeSOD collection:

@BernieTheBernie said in CodeSOD collection:

But I do not understand the library well enough yet to explain the difference in the beahvior.

This really doesn't have anything to do with the library in particular. It's just how C# works, or how any other language would for that matter.

Hm. I'll have to read that up, then.
Actually, I thought that

Option.Match(SomeMethod(some), NoneMethod());

is equivalent to

if (Option.IsSome()) { 
      SomeMethod(some); 
} else {
      NoneMethod();
}

and thus the NoneMethod would not at all be executed in the Some context.
Just like

a = b() || c();

c() won't be executed when b() is already true.

Well, I'll browse the webz to learn those thingies.

dkf

@BernieTheBernie said in CodeSOD collection:

Actually, I thought that
Option.Match(SomeMethod(some), NoneMethod());
is equivalent to
if (Option.IsSome()) { 
      SomeMethod(some); 
} else {
      NoneMethod();
}
and thus the NoneMethod would not at all be executed in the Some context.

That won't work because arguments are evaluated (at least immediately) before the call. It'd have to be more like:

Option.Match(() => SomeMethod(some), () => NoneMethod());

Zecc

@BernieTheBernie said in CodeSOD collection:

Actually, I thought that

Option.Match(SomeMethod(some), NoneMethod());

is equivalent to

if (Option.IsSome()) { 
      SomeMethod(some); 
} else {
      NoneMethod();
}

But it can't be. Option.Match(..., ...), regardless of what it does, is a plain old function call.
Its arguments need to be evaluated first, no matter what

The only difference to when you write this:

Option.Match(() => SomeMethod(some), () => NoneMethod());

...is that while both () => SomeMethod(some) and () => NoneMethod() arguments are evaluated before the function call, this time around they are first order functions; representations of blocks of code.
Option.Match then takes these and chooses to evaluate their bodies when and if it chooses.

Zecc

Let me write this to help drive the point home. (I'm making some informed assumptions about the library based on the code you posted above)

IOrchestra result = Orchestra.Match(
  Some: o => _Repository.UpdateOrchestra(o, Name, _Homepage, _Info),
  None:  _Repository.AddOrchestra(Name, _Homepage, _Info));
  
// is equivalent to

// Define a function that takes an IOrchestra, calls _Repository.UpdateOrchestra on it, and returns the IOrchestra returned by it
Func<IOrchestra,IOrchestra> arg1 =
    new Func<IOrchestra,IOrchestra>( (IOrchestra o) => _Repository.UpdateOrchestra(o, Name, _Homepage, _Info) );
    
// Run AddOrchestra immediately and stored the IOrchestra it returns
IOrchestra arg1 =
    _Repository.AddOrchestra(Name, _Homepage, _Info);
    
IOrchestra result = Orchestra.Match(Some: arg1, None: arg2);

while

IOrchestra result = Orchestra.Match(
  Some: o => _Repository.UpdateOrchestra(o, Name, _Homepage, _Info),
  None: () => _Repository.AddOrchestra(Name, _Homepage, _Info));
  
// is equivalent to

// Define a function that takes an IOrchestra, calls _Repository.UpdateOrchestra on it, and returns the IOrchestra returned by it
Func<IOrchestra,IOrchestra> arg1 = // Define a new function that takes an IOrchestra and returns and IOrchestra
    new Func<IOrchestra,IOrchestra>( (IOrchestra o) => _Repository.UpdateOrchestra(o, Name, _Homepage, _Info) );
    
// Define a function that doesn't take anything, but calls _Repository.AddOrchestra and returns the IOrchestra it returns
Func<IOrchestra> arg1 = 
    new Func<>( () => _Repository.AddOrchestra(Name, _Homepage, _Info) );
    
IOrchestra result = Orchestra.Match(Some: arg1, None: arg2);

Note here I'm assuming there's more than one overload of Orchestra.Match(), for various combinations of:

using a function that calculates an IOrchestra on the fly, and
using a fixed precalculated IOrchestra.

Tsaukpaetra

@Tsaukpaetra said in WTF is happening with Windows 10? And nothing else:

Status: Ah, yes, I remember now...

Gotta add some "allow some amount of time for the timestamps to mismatch" to the code now...

This is apparently incorrect. My assumption that total seconds is an absolute value may be wrong...

BernieTheBernie

@Zecc Thanks for your hints.
Slipping into confusion causes even more slips into further confusion...
But I some when saw the "parameter" issue (i.e. a parameter is evaluated before it is passed to the function), and the hidden cast of T to Func<T> - the last one is the decisive point here, done somewhere in the library.

dkf

@BernieTheBernie said in CodeSOD collection:

the hidden cast of T to Func<T>

Hidden casts? Are you writing C++? No? Then don't do that.

Zecc

@Zecc said in CodeSOD collection:

input1.Remove(input1.LastIndexOf(""));

I've confirmed: this will remove the last character of input1, throwing a ArgumentOutOfRangeException if empty.

Update on this: looking at the data this program is supposed to process, input is 100% of the time an empty string!

I really hope this code I recovered was not the most recent version of the program (version control? what's that?).
In any case, this was for a one-off and I've ended replacing the whole thing with a <200 LOC script, which took less time to write from near-scratch than auditing this legacy code.

boomzilla

@Zecc

bobjanova

@BernieTheBernie
the hidden cast of T to Func<T>

I'm going to need a pretty convincing explanation as to how that isn't a WTF.

bobjanova

@dkf There are places where implicit casts are great. If you have a complex number class, or a factional arithmetic one, or a big integer one, being able to do

Complex c = new Complex(3, 2) + 2; // implicit cast and operator overload for Complex +(Complex a, Complex b)

or even just

Complex c = 2;

is really nice.

It is kind of C++ like in that it's a powerful tool that can be abused to do horrible things, but it does have its place. It's hard to think of many outside mathematical algorithms, though, to be sure.

Tsaukpaetra

@boomzilla said in CodeSOD collection:

@Zecc

Halt! The Kernel would like a word with you!

Tsaukpaetra

@bobjanova said in CodeSOD collection:

It is kind of C++ like in that it's a powerful tool that can be abused to do horrible things, but it does have its place. It's hard to think of many outside mathematical algorithms, though, to be sure.

I like to initialize a datagram message with an empty string using the conversion override.