Ayende @ Rahien

I tell you, that thing is a bona fide ZEBRA, or a tale of being utterly stupid

Tue, 09 Aug 2016 09:00:00 GMT

We run our test suite in a loop to discover any race conditions, timing issues, errors, etc. When doing so, we got a hard crash from the dotnet.exe, and investigating the issue produced a stack trace inside the GC.

So I took a dump of the process memory, and created an issue about that with the CoreCLR repository, while giving it a very high priority internally, and having someone look at that very closely. We are using unsafe code extensively, so it was either a real GC bug or we messed up somewhere are corrupted our own state.

Very quickly Jan Kotas was able to point out that it was a heap corruption issue as well as the likely avenues for investigation.

After looking at this, we found that the problem was in our tests. In particular, in one specific test. In order to test the memory corruption, we changed it to add markers on where it overwrote the buffer, and the test passed.

This caused us additional concern, because the only thing we could think about was that maybe there is some invariant that is being broken. Our suspicion focused on the fixed statement in C# not working properly. Yes, I know, “hoof beats, horses, not zebras”.

So I went to the issue again and reported my finding, and Andy Ayers was kind enough to find the problem, and point it to me.

Here is the relevant test code:

This is during debugging, so you can see what the problem is. We defined size to be 40, and we defined an input buffer, whose size is 100.

A little bit below, we created an output buffer based on the size variable (40), and then wrote to it with the expected size of input.Length, which is 100. Everything behaved as it should, and we had a buffer overrun in the test, the heap was corrupted, and sometimes the GC died.

Also, I feel very stupid about spouting all sort of nonsense about bugs in the CLR when our code is unable to do simple arithmetic.

The good news, the bug was only in the tests, and the kind of support that you get from Microsoft on the CoreCLR is absolutely phenomenal. Thank you very much guys.

Answering a code test like a literature pop quiz

Tue, 09 Feb 2016 10:00:00 GMT

We get some… fascinating replies from candidates to our code tests. Some of them are so bad that I really wish that I could revoke some people’s keyboard access:

Case in point, we had a promising candidate from Israel’s equivalent of MIT (Technion, which is in the top 25 engineering schools in the world).

He submitted code that went something like this:

var nameFirstChar = char.Parse(name.Substring(0,1).ToLower());

switch (nameFirstChar)
{                                
    case 'a':
        using (StreamWriter indexFile = new StreamWriter(Path.Combine(basePath, "Name_IndeX_A.csv"), true))
        {
            indexFile.WriteLine(name);
        }
        break;
    case 'b':
        using (StreamWriter indexFile = new StreamWriter(Path.Combine(basePath, "Name_IndeX_B.csv"), true))
        {
            indexFile.WriteLine(name);
        }
        break;
    // ...
    // you can guess :-(
    // ...
    case 'y':
        using (StreamWriter indexFile = new StreamWriter(Path.Combine(basePath, "Name_IndeX_Y.csv"), true))
        {
            indexFile.WriteLine(name);
        }
        break;
    case 'z':
        using (StreamWriter indexFile = new StreamWriter(Path.Combine(basePath, "Name_IndeX_Z.csv"), true))
        {
            indexFile.WriteLine(name);
        }
        break;
}

And yes, he had the full A-Z there. This was part of a 1475 lines methods. And no, he didn’t handle all the cases for Unicode.

Yes, he just graduated, but some things are expected. Like knowing about loops.

How to ensure that you won’t get hired, quickly

Tue, 19 Jan 2016 10:00:00 GMT

I’m currently reviewing CVs, seemingly by the hundreds*. And I run into a guy which has a Github profile link in the CV. Such links are always followed, because seeing someone’s actual work is so much better than just reading some document about it.

But then I saw this:

And looking into the actual repository we have:

While this isn’t quite enough to give you a Darwin Award in the job hunting department (sadly, I saw worse), how could anyone think that having a publicly visible repository that says “I do illegal things to software” is a good idea. Leaving aside that you link that from your CV.

* It isn’t that many, it is just annoying.

Production postmortem: The case of the man in the middle

Fri, 14 Aug 2015 09:00:00 GMT

One of the most frustrating things when you dealing with production issues is when the problem is not in our product, but elsewhere. In particular, this post is dedicated to the hard work done by many anti virus products, in particular, to make our life harder.

Let us take a look at the following quote, taken from the ESET NOD32 Anti Virus knowledge base (emphasis mine):

By default, your ESET product automatically detects programs that are used as web browsers and email clients, and adds them to the list of programs that the internal proxy scans. This can cause loss of internet connectivity or other undesired results with applications that use network features but are not web browsers/email clients.

Yes, it can. In fact, it very often does.

Previously, we looked at a similar issue with Anti Virus slowing down I/O enough to cause us to slowly die. But in this case, the issue is a lot more subtle.

Because it is doing content filtering, it tends to put a much higher overhead on the system resources, which means that as far as the user is concerned, RavenDB is slow. We actually developed features specifically to handle this scenario. The traffic watch mode will tell you how much time you spend on the server side, and we have added a feature that will make RavenDB account for the internal work each query is doing, so we can tell where the actual cost is.

You can enable that by issuing:

GET databases/Northwind/debug/enable-query-timing

And one that is setup, you can get a good idea about what is costly in the query, as far as RavenDB is concerned. Here is an example of a very slow query:

You can see that the issue is that we are issuing a very wide range query, so most of the time is spent in inside Lucene. Other examples might be ridicilously complex queries, which result in high parsing time (we have seen queries in the hundreds of KB range). Or loading a lot of big documents, or… you get the drift. If we see that the server thinks that a query is fast, but the overall time is slow, we know to blame the network.

But an even more insidious issue is that this would drop requests, consistently and randomly (and yes, I know that those are contradictions, it was consistently dropping requests in a random pattern that seemed explicitly designed to thwart figuring out what is going on). Leading to things breaking, and escalated support calls. “RavenDB is broken” leads to a lot of headache, and a burning desire to hit something when you figure out that not only isn’t it your fault, but the underlying reason is actively trying to prevent you from figuring it out (I assume it is to deal with viruses that try to shut it off), which lead to really complex find facting sessions.

That is more annoying because it seems that the issue there was a bug in respecting keep alive sessions for authenticated requests under some scenarios, in the AV product in question! Absolutely not fun!

Is select() broken? Memory mapped files with unbufferred writes == race condition?

Sat, 16 Nov 2013 03:03:00 GMT

Let me start this post by stating that I am not even sure if what I am trying to do is legal here. But from reading the docs, it does appear to be a valid use of the API, and it does work, most of the time.

The full code can be found here: https://gist.github.com/ayende/7495987

The gist of it is that I am trying to do two things:

Write to a file opened with FILE_FLAG_WRITE_THROUGH | FILE_FLAG_NO_BUFFERING.
Read from this file using a memory map.
Occasionally, I get into situations where after I wrote to the file, I am not reading what I wrote.

I have a repro, and we reproduced this on multiple machines. Both Windows 7 and Windows 8.

Here is the relevant code (the full code is in the link), explanation on it below:

   1: const uint nNumberOfBytesToWrite = 4096*3;

   2: var buffer = (byte*)(VirtualAlloc(IntPtr.Zero, new UIntPtr(nNumberOfBytesToWrite), AllocationType.COMMIT, MemoryProtection.READWRITE)

   3:             .ToPointer());

4:

   5: for (int i = 0; i < nNumberOfBytesToWrite; i++)

   6: {

   7:     *(buffer + i) = 137;

   8: }

9:

  10: var g = Guid.NewGuid().ToString();

11:

  12: var safeHandle = CreateFile(g,

  13:     NativeFileAccess.GenericRead | NativeFileAccess.GenericWrite,

  14:     NativeFileShare.Read, IntPtr.Zero,

  15:     NativeFileCreationDisposition.OpenAlways,

  16:     NativeFileAttributes.Write_Through | NativeFileAttributes.NoBuffering | NativeFileAttributes.DeleteOnClose,

  17:     IntPtr.Zero);

18:

  19: var fileStream = new FileStream(safeHandle, FileAccess.ReadWrite);

  20: fileStream.SetLength(1024 * 1024 * 1024); // 1gb

21:

  22: if (safeHandle.IsInvalid)

  23: {

  24:     throw new Win32Exception();

  25: }

26:

  27: FileStream mms = fileStream;

  28: //mms = new FileStream(g, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete);

  29: var mmf = MemoryMappedFile.CreateFromFile(mms, Guid.NewGuid().ToString(), fileStream.Length,

  30:     MemoryMappedFileAccess.Read, null, HandleInheritability.None, true);

31:

  32: MemoryMappedViewAccessor accessor = mmf.CreateViewAccessor(0, fileStream.Length, MemoryMappedFileAccess.Read);

  33: byte* ptr = null;

  34: accessor.SafeMemoryMappedViewHandle.AcquirePointer(ref ptr);

35:

  36: Task.Factory.StartNew(() =>

  37: {

  38:     long lastPos = 0;

  39:     while (true)

  40:     {

  41:         int count = 0;

  42:         while (true)

  43:         {

  44:             if (*(ptr + lastPos) != 137)

  45:             {

  46:                 break;

  47:             }

  48:             lastPos += 4096;

  49:             count ++;

  50:         }

  51:         Console.WriteLine();

  52:         Console.WriteLine("Verified {0} MB", count * 4 / 1024);

  53:         Console.WriteLine();

  54:         Thread.Sleep(2000);

  55:     }

  56: });

57:

  58: for (int i = 0; i < 1024*64; i++)

  59: {

  60:     var pos = i*nNumberOfBytesToWrite;

  61:     if (i%100 == 0)

  62:         Console.Write("\r{0,10:#,#} kb", pos/1024);

  63:     var nativeOverlapped = new NativeOverlapped

  64:     {

  65:         OffsetHigh = 0,

  66:         OffsetLow = (int) pos

  67:     };

68:

  69:     uint written;

  70:     if (WriteFile(safeHandle, buffer, nNumberOfBytesToWrite, out written, &nativeOverlapped) == false)

  71:         throw new Win32Exception();

72:

  73:     for (int j = 0; j < 3; j++)

  74:     {

  75:         if (*(ptr + pos) != 137)

  76:         {

  77:             throw new Exception("WTF?!");

  78:         }

  79:         pos += 4096;

  80:     }

  81: }

This code is doing the following:

We setup a file handle using NoBuffering | Write_Through, and we also map the file using memory map.
We write 3 pages (12Kb) at a time to the file.
After the write, we are using memory map to verify that we actually wrote what we wanted to the file.
_At the same time_ we are reading from the same memory in another thread.
Occasionally, we get an error where the data we just wrote to the file cannot be read back.

Now, here is what I think is actually happening:

When we do an unbuffered write, Windows has to mark the relevant pages as invalid.
I _think_ that it does so before it actually perform the write.
If you have another thread that access that particular range of memory at the same time, it can load the _previously_ written data.
The WriteFile actually perform the write, but the pages that map to that portion of the file have already been marked as loaded.
At that point, when we use the memory mapped pointer to access the data, we get the data that was there before the write.

As I said, the code above can reproduce this issue (you might have to run it multiple times).

I am not sure if this is something that is valid issue or just me misusing the code. The docs are pretty clear about using regular i/o & memory mapped i/o. The OS is responsible to keeping them coherent with respect to one another. However, that is certainly not the case here.

It might be that I am using a single handle for both, and Windows does less checking when that happens? For what it is worth, I have also tried it using different handles, and I don’t see the problem in the code above, but I have a more complex scenario where I do see the same issue.

Of course, FILE_FLAG_OVERLAPPED is not specified, so what I would actually expect here is serialization of the I/O, according to the docs. But mostly I need a sanity check to tell me if I am crazy.

And some people will INSIST on shooting them own foot off

Fri, 14 Jun 2013 09:00:00 GMT

Because, clearly, that is what is missing. RavenDB GetAll extension method

Don’t play peekaboo with support, damn it!

Tue, 20 Nov 2012 10:00:00 GMT

One of the things that we really pride ourselves with Hibernating Rhinos is the level of support. Just to give you some idea, today & yesterday we had core team members on the phone with people (not customers, yes) who are having problems with RavenDB for quite some time.

Now, I understand that you may not always have the information to give, but what you have, give me! So I can help you.

From a recent exchange in the mailing list:

var clickCount = session.Query<TrackerRequest>().Where(t => t.TrackerCreated == doc.Created).Where(t => t.Type == Type.Click).Count();
This gives:
"Non-static method requires a target"

To which I replied:

What is the full error that you get? Compile time? Runtime?

The answer I got:

This is a document I'm trying to 'Count':
{
"Created": "2012-11-15T16:12:42.1775747",
"IP": "127.0.0.1",
"TrackerCreated": "2012-11-15T14:12:16.3951000Z",
"Referrer": "http://example.com",
"Type": "Click"
}
Raven terminal gives:
Request # 172: GET     -     3 ms - <system>   - 200 - /indexes/Raven/DocumentsByEntityName?query=Tag%253ATrackerRequests&start=0&pageSize=30&aggregation=None&noCache=-1129797484
        Query: Tag:TrackerRequests
        Time: 2 ms
        Index: Raven/DocumentsByEntityName
        Results: 3 returned out of 3 total.

By the way, you might note that this ISN’T related in any way to his issue. This query (and document) were gotten from the Studio. I can tell by the URL.

Then there was this:

I mean, seriously, I am happy to provide support, even if you aren’t a customer yet, but don’t give me some random bit of information that has absolutely nothing to do to the problem at hand and expect me to guess what the issue is.

Relevant information like the stack trace, what build you are on, what classes are involved, etc are expected.

Negative hiring decisions, Part I

Thu, 22 Sep 2011 09:00:00 GMT

One of the things that I really hate is to be reminded anew how stupid some people are. Or maybe it is how stupid they think I am. One of the things that we are doing during interviews is to ask candidates to do some fairly simple code tasks. Usually, I give them an hour or two to complete that (using VS and a laptop), and if they don’t complete everything, they can do that at home and send me the results.

This is a piece of code that one such candidate has sent. To be clear, this is something that the candidate has worked on at home and had as much time for as she wanted:

public int GetTaxs(int salary)
{
    double  net, tax;

    switch (salary)
    {
        case < 5070:
            tax = salary  * 0.1;
            net=  salary  - tax ;
            break;

        case < 8660:
        case > 5071:
            tax = (salary - 5071)*0.14;
            tax+= 5070 * 0.1;
            net = salary-tax;   
            break;
        case < 14070:
        case > 8661:
            tax=  (salary - 8661)*0.23;
            tax+= (8661 - 5071 )*0.14;
            tax+= 5070 *0.1;
            net=  salary - tax;
            break;
        case <21240:
        case >14071:
            tax=  (salary- 14071)*0.3;
            tax+= (14070 - 8661)*0.23;
            tax+= (8661 - 5071 )*0.14;
            tax+= 5070 *0.1;
            net= salary - tax;
            break;
        case <40230:
        case >21241:
            tax=  (salary- 21241)*0.33;
            tax+= (21240 - 14071)*0.3;
            tax+= (14070 - 8661)*0.23;
            tax+= (8661 - 5071 )*0.14;
            tax+= 5070 *0.1;
            net= salary - tax;
            break;
        case > 40230:
            tax= (salary - 40230)*0.45;
            tax+=  (40230- 21241)*0.33;
            tax+= (21240 - 14071)*0.3;
            tax+= (14070 - 8661)*0.23;
            tax+= (8661 - 5071 )*0.14;
            tax+= 5070 *0.1;
            net= salary - tax;
            break;
        default:
            break;
    }
}

Submitting code that doesn’t actually compiles is a great way to pretty much ensures that I won’t hire you.

Tales from a code review gone wrong

Thu, 24 Feb 2011 10:00:00 GMT

Originally posted at 2/23/2011

Yes, I know that the NHibernate documentation says that you should override Equals and GetHashCode, but I get the feeling that this wasn’t what the documentation authors intended:

You saved 5 cents, and your code is not readable, congrats!

Fri, 22 Oct 2010 10:00:00 GMT

I found myself reading this post, and at some point, I really wanted to cry:

We had relatively long, descriptive names in MySQL such as timeAdded or valueCached. For a small number of rows, this extra storage only amounts to a few bytes per row, but when you have 10 million rows, each with maybe 100 bytes of field names, then you quickly eat up disk space unnecessarily. 100 * 10,000,000 = ~900MB just for field names!

We cut down the names to 2-3 characters. This is a little more confusing in the code but the disk storage savings are worth it. And if you use sensible names then it isn’t that bad e.g. timeAdded -> tA. A reduction to about 15 bytes per row at 10,000,000 rows means ~140MB for field names – a massive saving.

Let me do the math for a second, okay?

A two terabyte hard drive now costs 120 USD. By my math, that makes:

1 TB = 60 USD
1 GB = 0.058 USD

In other words, that massive saving that they are talking about? 5 cents!

Let me do another math problem, oaky?

Developer costs about 75,000 USD per year.

(52 weeks – 2 vacation weeks) x 40 work hours = 2,000 work hours per year.
75,000 / 2,000 = 37.5 $ / hr
37.5 / 60 minutes = 62 cents per minutes.

In other words, assuming that this change cost a single minute of developer time, the entire saving is worse than moot.

And it is going to take a lot more than one minute.

Update: Fixed decimal placement error in the cost per minute. Fixed mute/moot issue.

To those of you pointing out that real server storage space is much higher. You are correct, of course. I am trying to make a point. Even assuming that it costs two orders of magnitudes higher than what I said, that is still only 5$. Are you going to tell me that saving the price of a single cup of coffee is actually meaningful?

To those of you pointing out that MongoDB effectively stores the entire DB in memory. The post talked about disk size, not about memory, but even so, that is still not relevant. Mostly because MongoDB only requires indexes to fit in memory, and (presumably) indexes don't really need to store the field name per each indexed entry. If they do, then there is something very wrong with the impl.

How to pay 3 times for the same flight ticket

Fri, 13 Aug 2010 19:39:00 GMT

My current mood is somewhere between mad, pissed off, frustrated and exhausted.

All I want to do it arrive home, but dark forces has conspired against me. It works like this:

I got a ticket both ways to London, and I left to London quite happily, taking the train to the airport. At that point things started going horribly wrong. An idiot has suicided on the train trucks, leading to about 2 hours of delays, which, naturally, caused me to miss my flight. We will leave that aside and just point out that I somehow managed to get to London without being late to my course the next morning.
That is when I heard that since I was a no show for the outgoing flight, the airline cancelled my flight back. I had to order a new one, but because I was busy actually doing the course, I asked the company to order that for me.
They did, and I went to the airport quite satisfied, except that on arrival, the stupid machine told me that my booking is invalid, which is the point that I discovered that the ticket was ordered on Ayende Rahien. Unfortunately, legally speaking, that guy doesn’t exists. The airline was quite insistent that they can’t put me on board with the different name. And they made me buy a new ticket.

That was the point (after I paid for the same bloody seat for the third time) that they told me that they oversold the flight, and that they put me on waiting list.

I tried to explain that they could use the second ticket that I bought, and just give me the same seat, but they told me that they already sold that to someone else.

I am currently waiting to hear whatever I’ll get to go home tonight or not.

Your truly,

Pissed off,

Oren Eini (because being Ayende is currently dangerous, given my mood)

WTF?! Google Groups really hates me!

Fri, 04 Jun 2010 05:26:00 GMT

There are no other owners for the group, I have no idea what is going on…

Update: Looks like Google disabled my account, for reasons known only to it. I had to go through a “send me an SMS to activate it” loop, but now it is working.

Some code just make me queasy

Fri, 14 May 2010 19:41:00 GMT

And this one hits a lot of the high notes in that regard:

Three properties to say the same thing?!

Help requests that make me want to cry

Fri, 21 Aug 2009 00:27:00 GMT

This showed up on my mail box, with the following text:

When i try to insert an item to db, VS gives a stackoverflow exception. is it because of that i am using generic classes??

I am not going to comment further, I am stepping away from the keyboard and going to have a lie down.

NHibernate – Get Thou Out Of My Database

Sat, 06 Jun 2009 12:13:00 GMT

There are evil people in this world, and some of them want access to my database. Unfortunately, they are often part of that nasty integration team and they try to integrate directly into my database. I tried beating them with clubs and lobbing arguments about letting other people mess with my implementation details, but they have been persistent. That is when I reached out to a technological solution for the problem.

I want to emphasize that this is the nuclear option and you want to really consider it before going that route.

We are going to use NHibernate to do that, naturally. Here is how:

 

Which results in the following schema:

create table [tbl_-1434067361] (
   [col_287061521] INT IDENTITY NOT NULL,
   [col_4699698] INT not null,
   [col_-1966747349] NVARCHAR(255) null,
   [col_-649855325] NVARCHAR(255) null,
   [col_-649855326] NVARCHAR(255) null,
   [col_775690683] NVARCHAR(255) null,
   [col_-2127361396] NVARCHAR(255) null,
   [col_-1334581412] NVARCHAR(255) null,
   primary key ([col_287061521])
)

To make sure that we follow procedure, we are even using the naming convention of the organization! Lovely, isn’t it? It is obvious that this is my people table, right?

All arguments against this schema can be answered using: “It is more secured”.

It might be the nuclear option, but it tend to work :-)

Nitpicker corner: No, I don’t suggest you would do this, that is why the code is an image. This is just an example of how to programmatically modify the configuration and mapping. I only used it once, during a demo to the integration people, to get them to leave my DB alone. If you push something like that for production you should probably be shot.

PayPal: It may be your account, but you aren’t going to use it

Fri, 15 May 2009 15:43:09 GMT

In general, I have been very happy with PayPal over the years. Recently, however, I have become quite frustrated with it. I am using it quite often to send and accept money to people I am working with, and for a while now, I have been unable to do so.

Apparently, my actions has been caught by some automated security systems and they decided to ban any transactions on my account. That is actually acceptable from my point of view. I feel better knowing that there are active attempts to protect me from fraud or theft.

What I don’t find acceptable is that when I called PayPal to try to resolve that, the response that I got was: It is an act of God, nothing we mere mortals can do about it.

I understand all about false positives, and aside from the annoyance factor, they are acceptable if they aren’t happening very often. What I am not able to comprehend is responses such as: No one in PayPal can help you. And that is after I already escalated to a supervisor!

I recorded my conversation with PayPal, you can download it here or just listen:

I find the whole thing utterly infuriating, and totally unacceptable.

Kobe – Architectural Overview

Fri, 17 Apr 2009 07:57:55 GMT

Sometimes, you have to make a distinction between the system architecture and the system implementation. It is possible to have a great architecture and have the implementers butcher it by not paying attention at some points.

Sometimes, the problem in the code implementation is so bad that it becomes an architectural problem. Kobe is one such example.

We can start with the pervasive Primitive obsession that is all over the map. The common use of “string thisIsReallyGuid”, Copy & paste programming, horrible Exception Handling and total lack of respect for the fundamentals of OO.

All of those tie back together to the lack of architectural oversight over the project. I have the feeling that at no point someone stopped and said: “there has got to be a better way of doing this.”

DRY is a true principle in development, it transcend programming languages and paradigms, about the only one that I can think of that does so. If you violate DRY, you are in for a world of trouble, sooner than later.

SOLID is a set of principles used to build good OO applications. If you are using OO language and paradigms and fail to pay attention to SOLID, it will hurt you at some point. Not as soon as if you have violated DRY, but soon enough.

8.5% of Kobe is copy & pasted code. And that is with the sensitivity dialed high, if we set the threshold to 3, which is what I commonly do, is goes up to 12.5%. A lot of the duplicated code relates to exception handling and caching. Those are classic cases for using an aspect to handle cross cutting concerns. That would reduce a lot of the duplicated code.

The other problems that I have with Kobe is its total lack of respect for SOLID principles. You know what, let us throw solid out the window, I am going to focus on a single principle, Single Responsibility Principle. Just about any method that you care to name does more than one thing, in many cases, they do a lot more than one thing.

This is absolutely wrong.

The “repositories” in Kobe has so many responsibilities that they are just shy of having a singularity around them.

Overall, Kobe feels very amaturish, as if it was written without true understanding of what is expected of quality code, and without really understanding the underlying platform, frameworks or the infrastructure that they are using.

I don’t like it at all, and it is a horrible sample application. In my considered opinioned, it should be pulled from the site, rewritten, and only then published again.

Kobe – an example of exception handling done wrong

Fri, 17 Apr 2009 07:46:35 GMT

In Kobe, pretty much all exception handling is this:

catch (Exception ex)
{
    bool rethrow = ExceptionPolicy.HandleException(ex, "Service Policy");
    if (rethrow && WebOperationContext.Current == null)
    {
        throw;
    }
    return null;
}

These lines appear in just about any method in the application.

They are wrong. In fact, they are wrong on more than one level. Violating DRY is a big offence, but even ignoring that, they are still wrong.

Exception handling is not something that you should take lightly, and there is a strong tie between the way that you write the code and the way errors are handled. The whole purpose of this exception handling is to avoid throwing an exception by setting some configuration parameter.

The problem with this is that this purpose is wrong. This is a case of writing the code without actually trying to understand what the implications are. You cannot do this sort of thing, because the calling code never expects a null, it expect a valid result or an exception.

This approach bring us back to the bad old days of error codes. Not to mention that even if we actually checked for a null return value, what the hell is the error that actually caused this to happen. Oh, it was put in the log, so now we need to go and look at the log file and try to match timestamps (if we even can).

Exception handling should appear in exactly two places:

When an error is expected (making a web request call, for example) and there is some meaningful behavior to be done in the case of a failure (such as retrying after some delay)
On a system boundary, in which case you need to make a decision about how you are going to expose the error to the outside world.

It is important to note that I explicitly used the term system boundary, instead of service boundary, which is more commonly used. Exception handling within a system should not try to hide errors. Error should propagate all the way to the system boundary, because otherwise we lose a lot of important information.

System boundary is defined as whatever is visible to the actual user or other systems.

Oh, and exception handling in the system boundary is a system infrastructure problem, it is not something that you have to write code for all over the place.

Kobe – When the documentation is the only delivery that matters

Fri, 17 Apr 2009 07:23:44 GMT

When comparing the Kobe code base to the Kobe documentation, I am starting to get a sinking feeling. The documentation is really nice, the code is anything but.

When you put out guidance, the documentation is nice, but it is the code that people are going to look it, learn and try to emulate. And when you put out something that doesn’t meet basic parameters for good software, that is a huge problem.

With Oxite, Microsoft had the excuse of it being slipped out without anyone noticing. This time it is official, it has passed the proper review procedure, and is explicitly marketed as “intended to guide you with the planning, architecting, and implementing of Web 2.0 applications and services.”

Sorry, that is entirely unacceptable.

Putting something like that out there as guidance is actively doing harm to the community as a whole. It means that people who would look at that and try to use what they see there are going to get bitten by bad practices, bad architecture and overly complex and redundant approaches.

Phil Haack said that: “I was part of a high-level review and I felt most of the app that I saw was reasonably put together.”

I submit that high level review of such guidance packages is absolutely not sufficient. You have to get people to do a full review cycle of that, which include the code, the documentation, the reasoning and anything else that is going out there.

Kobe – Data Access done wrong

Fri, 17 Apr 2009 06:45:08 GMT

From the Kobe documentation:

The Repository pattern is applied to mediate access to the data store through a Repository provider.

From PoEAA:

A system with a complex domain model often benefits from a layer, such as the one provided by Data Mapper (165), that isolates domain objects from details of the database access code. In such systems it can be worthwhile to build another layer of abstraction over the mapping layer where query construction code is concentrated. This becomes more important when there are a large number of domain classes or heavy querying. In these cases particularly, adding this layer helps minimize duplicate query logic.

I think that Repository is a fad, which is why I don’t use it much anymore myself, and looking at the usage semantics of the “repositories” in Kobe, they certainly don’t fit the bill.

Repositories are here to encapsulate query logic, that is not the case with Kobe. Here is a partial view of some of the repositories that it have:

In general, looking at the any of the repositories in detail, I really want to cry. I mean, just take a look:

More to the point, let us look at GroupRepository.AcceptGroupInvite (I removed all the copy & paste crap to concentrate on the actual code):

GroupInvite groupInvite = _context.GroupInviteSet
                                 .Where(gi => gi.GroupInviteId == groupInviteId)
                                 .FirstOrDefault();

if (groupInvite == null)
    throw new DataException("Invalid Invite Id", null);

groupInvite.InvitationStatuses = _context.InvitationStatusSet
                                        .Where(s => s.InvitationStatusName == "Accepted")
                                        .FirstOrDefault(); ;

_context.SaveChanges(true);

There are so many things wrong with this approach that I don’t even know where to start.

You can see how the architects or developers resented having to use an ORM. If they couldn’t have stored procedures in the database, then by Jove and his keyboard, they will write stored procedures in code.

Kobe uses Entity Framework for data access, and they treat it as if it was a dataset. As an OR/M aficionado, I am insolated. This shows complete lack of understanding how Entity Framework work, doing a lot more work than you should, brute force & big hammer approach.

From an architectural perspective, most of what Kobe is doing is calling down to the repository to perform the data access details. Business logic, such as there is, is spread between the data access, the controllers and the service layer, in such a way that make is very hard to have a good idea about what is actually happening in the application.

Data access details are quite pervasive throughout the application, and the whole feeling of the application is of a circa 2001 sample app using stored procedures and hand rolled data access layers.