Thursday, January 25, 2024

Never, ever make your Random static

Years ago it was popular to advocate for sharing the instance of the Random class. There are numerous sources, including blogs, the stack, etc. Ocasionally, someone dared to write that it could not be the best idea. But still, looking for information, people stumle across this decade-old recommendation.
That was the case of one of our apps. In one critical place, a shared instance of random was used

public static Random _rnd = new Random();

...
..._rnd.Next()...
This worked. For years. Until one day, it failed miserably.
You see, the random has two internal variables it overwrites each time Next is called so that it can advance to the next random value. If your app is just winning a lottery ticket in its concurrent execution, the two values can end up being overwritten with the same value.
And guess what, random starts to return 0 each time Next is called! For us, it was the case of an internal stress test, where a heavy traffic was directed onto the application but it can happen just anytime.
There are numerous solutions. First, starting from .NET6 there's the Random.Shared static property, marked as thread-safe. In older frameworks, one of alternative approaches should be used, e.g. this

Wednesday, January 24, 2024

FireFox slowly becomes a new Internet Explorer

For years in the past, developing web apps was a constant case of if (IE) do_something_else_than_for_other_browsers(). Sadly, we lately have some bad cases where things seem similar, but instead of IE we now have FF.
One of earlier cases concerned the location.reload API. In a specific case of an iframe, the app calls this to reload the content when users change their color theme. Worked everywhere instead of FF. As seen in the docs, FF has its own forceGet parameter, not supported in the spec but mentioned in the docs. Seems that location.reload works for us in FF only when this extra argument is provided.
Another case appeared lately, unfortunately. Take the WS-Federation protocol and consider a scenario where the identity provider and the service provider are on different domains.
Signing in works correctly. The service provider redirects with wa=wsignin1.0 and the identity provider responds with the SAML token POSTed to the service provider.
Signing out is implemented using nested iframes where the identity provider points to the service provider and adds wa=wsignoutcleanup1.0 to terminate the session (drop the service provider session cookie). As you know, there's been a change lately in the way cross domain cookies are handled. To prevent the CSRF, SameSite flag was added and in the default case, the cookie falls back to SameSite=lax which prevents it from being accessed cross different domains.
There's however still a way to make cookies available in cross domain requests, you are supposed to just mark them with SameSite=none;Secure. And guess what, this works in all other browsers, except FF. It turns out, the default security settings for FF prevent all crossdomain cookies, no matter if they are marked with SameSite=none or not.
Sure, users can opt out by lowering the security level or configure exceptions for your app but this doesn't change the fact that the specific scenario mentioned above just doesn't work in the default FF setup. Other browsers have their own security settings and, at least at the very moment, you can opt in for more strict settings (and cross domain cookies don't work anymore) but this requires a change in default settings. In case of FF, it's the default setting that it's against the samesite spec.

Monday, January 15, 2024

An interesting edge case in C# yield state machines

Among many useful LINQ methods, there's no functional ForEach. People often implement it on their own, an extension method is just a few lines of code.
Someone did that in one of our projects, too:
public static void ForEach1<T>( this IEnumerable<T> enumeration, Action<T> action )
{
	foreach ( T item in enumeration )
	{
		action( item );
	}
}
This works great. Someone else, however, noticed that while this works great, it doesn't allow further chaining, since the method returns void
This someone else modified the code slightly:
public static IEnumerable<T> ForEach2<T>( this IEnumerable<T> enumeration, Action<T> action )
{
	foreach ( T item in enumeration )
	{
		action( item );
		yield return item;
	}
}
This doesn't work great. Frankly, it doesn't work at all. Surprising but true. Take this:
internal class Program
{
	static void Main( string[] args )
	{
		Test1();
		Test2();

		Console.ReadLine();
	}

	static void Test1()
	{
		var array = new int[] { 1,2,3 };

		var list = new List<int>();

		array.ForEach1( e => list.Add( e ) );

		Console.WriteLine( list.Count() );
	}
	static void Test2()
	{
		var array = new int[] { 1,2,3 };

		var list = new List<int>();

		array.ForEach2( e => list.Add( e ) );

		Console.WriteLine( list.Count() );
	}
}

public static class EnumerableExtensions
{
	public static void ForEach1<T>( this IEnumerable<T> enumeration, Action<T> action )
	{
		foreach ( T item in enumeration )
		{
			action( item );
		}
	}

	public static IEnumerable<T> ForEach2<T>( this IEnumerable<T> enumeration, Action<T> action )
	{
		foreach ( T item in enumeration )
		{
			action( item );
			yield return item;
		}
	}
}
While this is expected to produce 3 for both tests, it produces 3 and 0. The upgraded version doesn't work as expected.
The reason for this is a kind-of-a-limitation of the state machine generated by the yield sugar. The machine doesn't execute any code until the very result is enumerated. This means that changing
array.ForEach2( e => list.Add( e ) );
to
array.ForEach2( e => list.Add( e ) ).ToList();
would "fix it". What a poor foreach, though, that requires an extra terminator (the ToList) and doesn't work otherwise.
Luckily, a simpler fix exists, just forget the state machine at all for this specific case:
public static IEnumerable<T> ForEach2<T>( this IEnumerable<T> enumeration, Action<T> action )
{
	foreach ( T item in enumeration )
	{
		action( item );
	}

	return enumeration;
}