views:

1241

answers:

22

The IT department of a subsidiary of ours had a consulting company write them an ASP.NET application. Now it's having intermittent problems with mixing up who the current user is and has been known to show Joe some of Bob's data by mistake.

The consultants were brought back to troubleshoot and we were invited to listen in on their explanation. Two things stuck out.

First, the consultant lead provided this pseudo-code:

void MyFunction()
{
    Session["UserID"] = SomeProprietarySessionManagementLookup();
    Response.Redirect("SomeOtherPage.aspx");
}

He went on to say that the assignment of the session variable is asynchronous, which seemed untrue. Granted the call into the lookup function could do something asynchronously, but this seems unwise.

Given that alleged asynchronousness, his theory was that the session variable was not being assigned before the redirect's inevitable ThreadAbort exception was raised. This faulure then prevented SomeOtherPage from displaying the correct user's data.

Second, he gave an example of a coding best practice he recommends. Rather than writing:

int MyFunction(int x, int x)
{
    try 
    {
        return x / y; 
    }
    catch(Exception ex)
    {
        // log it
        throw;
    }
}

the technique he recommended was:

  int MyFunction(int x, int y, out bool isSuccessful)
  {
    isSuccessful = false;

    if (y == 0)
        return 0;

    isSuccessful = true;

    return x / y;
  }

This will certainly work and could be better from a performance perspective in some situations.

However, from these and other discussion points it just seemed to us that this team was not well-versed technically.

Opinions?

+18  A: 

Rule of thumb: If you need to ask if a consultant knows what he's doing, he probably doesn't ;)

And I tend to agree here. Obviously you haven't provided much, but they don't seem terribly competent.

Cody Brocious
Re "If you need to ask...", yeah, that's a good point!
marc
it looks like they demonstrated quite clearly that they are not competent - fire them and hire me instead ;-)
Steven A. Lowe
Often I've found that people who need to ask don't know what they are doing either. If you don't *know* that the guy is wrong, you don't know what you are doing either. :)
brian d foy
"if you need to ask ..." doesn't necessarily mean the guy doesn't know what he'd doing, but it does indicate a problem in your relationship with this consultant that need to be resolved.
John MacIntyre
+5  A: 

For the first point, that does indeed seem bizarre.

On the second one, it's reasonable to try to avoid division by 0 - it's entirely avoidable and that avoidance is simple. However, using an out parameter to indicate success is only reasonable in certain cases, such as int.TryParse and DateTime.TryParseExact - where the caller can't easily determine whether or not their arguments are reasonable. Even then, the return value is usually the success/failure and the out parameter is the result of the method.

Jon Skeet
Yeah, avoiding exceptions at all costs is just silly. They oftentimes make the code a lot easier to understand and thus easier to maintain, so trying to skirt around the exception like that to me smells of someone unnecessarily doing micro-performance optimization (a BAD THING)
Mike Stone
+12  A: 

If the consultant has written an application that's supposed to be able to keep track of users and only show the correct data to the correct users and it doesn't do that, then clearly something's wrong. A good consultant would find the problem and fix it. A bad consultant would tell you that it was asynchronicity.

Randy
Exactly. Who cares what the problem really is. The only real answer here is for the consultant to fix it.
Chris Lively
Well put. ************
Herb Caudill
+13  A: 

I would agree. These guys seem quite incompetent.

(BTW, I'd check to see if in "SomeProprietarySessionManagementLookup," they're using static data. Saw this -- with behavior exactly as you describe on a project I inherited several months ago. It was a total head-slap moment when we finally saw it ... And wished we could get face to face with the guys who wrote it ... )

John Rudy
Thanks for the input. They don't seem overly interested in what we think but I'll put it on the table. Thus far we've not been invited to look at any source code.
marc
Gosh it's your code, you don't need to be invited! :)
Matt Rogish
Agreed ... If ever refused to show code to a client who was paying for it, I'd get fired. And probably beaten up with a rubber chicken.
John Rudy
I think it's mostly our "child" organization that doesn't want to share the code - a turf thing...
marc
+1 Asynchronous is nonsense, whatever the method is doing, the Session variable is set synchronously. Static variables, possibly from a developer who's discovered the singleton pattern are the reason. Don't ask for source: look at the code with Lutz Reflector!
Joe
Reflector is a great idea!
John Rudy
Asynchronous might not be total nonsense, but the consultant-provided pseudocode is wrong, but it's pseudocode after all. See my answer for more info.
Robert Paulson
I think you're right on the money. For sure they're using static variables in their proprietary session crap.
MusiGenesis
I completely agree, saw the exact same behaviour when a developer thought he should build a static (in his opinion typed) wrapper class around the session object...
Cohen
I had to build a static variable based session manager before. It actually works better than one would guess. There are a couple of deployment concerns though.
Joshua
I've had singleton-based strong-typed session wrappers work without any multi-user issues (and we do test those!) -- it's all in how you access the session. However, in my original answer here, I was alluding to static page-level members, which almost always puke in multi-user.
John Rudy
BTW, if you ever need to replicate a multi-user issue and you don't have multiple testers, just use two different browsers -- say, IE and Firefox. Both will get unique sessions. Of course, clicking them out fast enough might be tricky. :)
John Rudy
+2  A: 

I agree with him in part -- it's definitely better to check y for zero rather than catching the (expensive) exception. The out bool isSuccessful seems really dated to me, but whatever.

re: the asynchronous sessionid buffoonery -- may or may not be true, but it sounds like the consultant is blowing smoke for cover.

Danimal
A valid point, but the code is still low quality. If you're going to allow a 0 divsor, then catch it before the division and log it, but don't count on someone writing client code to "do the right thing" when a false success flag is passed back to them. If you're going to require that client code reacts to the success flag, why not just require that client code not pass a 0 in the first place? If that were part of the interface contract, then an unhandled exception would be the proper way to handle div/zero.
Paul Keister
+1  A: 

Cody's rule of thumb is dead right. If you have to ask, he probably doesn't.

It seems like point two its patently incorrect. .NET's standards explain that if a method fails it should throw an exception, which seems closer to the original; not the consulstant's suggestion. Assuming the exception is accurately & specifically describing the failure.

Steve Duitsman
+1  A: 

The consultants created the code in the first place right? And it doesn't work. I think you have quite a bit of dirt on them already.

The asynchronous answer sounds like BS, but there may be something in it. Presumably they have offered a suitable solution as well as pseudo-code describing the problem they themselves created. I would be more tempted to judge them on their solution rather than their expression of the problem. If their understanding is flawed their new solution won't work either. Then you'll know they are idiots. (In fact look round to see if you have a similar proof in any other areas of their code already)

The other one is a code style issue. There are a lot of different ways to cope with that. I personally don't like that style, but there will be circumstances under which it is suitable.

Simon
A: 

On the second point, I would not use exceptions here. Exceptions are reserved for exceptional cases.
However, division of anything by zero certainly does not equal zero (in math, at least), so this would be case specific.

Alf Zimmerman
A: 

Typical "consultant" bollocks:

  1. The problem is with whatever SomeProprietarySessionManagementLookup is doing
  2. Exceptions are only expensive if they're thrown. Don't be afraid of try..catch, but throws should only occur in exceptional circumstances. If variable y shouldn't be zero then an ArgumentOutOfRangeException would be appropriate.
Duncan Smart
but an exception is indeed thrown when you try to divide by zero -- the call stack is wound up, etc.
Danimal
A: 

I have to agree with John Rudy. My gut tells me the problem is in SomeProprietarySessionManagementLookup().

.. and your consultants do not sound to sure of themselves.

phreakre
+1  A: 

They're wrong on the async.

The assignment happens and then the page redirects. The function can start something asynchronously and return (and could even conceivably alter the Session in its own way), but whatever it does return has to be assigned in the code you gave before the redirect.

They're wrong on that defensive coding style in any low-level code and even in a higher-level function unless it's a specific business case that the 0 or NULL or empty string or whatever should be handled that way - in which case, it's always successful (that successful flag is a nasty code smell) and not an exception. Exceptions are for exceptions. You don't want to mask behaviors like this by coddling the callers of the functions. Catch things early and throw exceptions. I think Maguire covered this in Writing Solid Code or McConnell in Code Complete. Either way, it smells.

Cade Roux
FYI, C# has indexers that can hide assignments as actual methods, and so the Session[]= call could potentially fire off an asynch assignment... but this is unbelievably unlikely, and easy to determine by looking at the documentation (this would NOT be hidden from the docs).
Mike Stone
I'm pretty sure he's talking about ASP.NET Session which doesn't behave that way.
Cade Roux
A: 

Storing in Session in not async. So that isn't true unless that function is async. But even so, since it isn't calling a BeginCall and have something to call on completion, the next line of code wouldn't execute until the Session line is complete.

For the second statement, while that could be used, it isn't exactly a best practice and you have a few things to note with it. You save the cost of throwing an exception, but wouldn't you want to know that you are trying to divide by zero instead of just moving past it?

I don't think that is a solid suggestion at all.

Tom
+8  A: 

On the asynchronous part, the only way that could be true is if the assignment going on there is actually an indexer setter on Session that is hiding an asynchronous call with no callback indicating success/failure. This would seem to be a HORRIBLE design choice, and it looks like a core class in your framework, so I find it highly unlikely.

Usually asynchronous calls have a way to specify a callback so you can determine what the result is, or if the operation was successful. The documentation for Session should be pretty clear though on if it is actually hiding an asynchronous call, but yeah... doesn't look like the consultant knows what he is talking about...


The method call that is being assigned to the Session indexer cannot be asynch, because to get a value asynchronously, you HAVE to use a callback... no way around that, so if there is no explicit callback, it's definitely not asynch (well, internally there could be an asynchronous call, but the caller of the method would perceive it as synchronous, so it is irrelevant if the method internally for example invokes a web service asynchronously).


For the second point, I think this would be much better, and keep the same functionality essentially:

int MyFunction(int x, int y)
{
    if (y == 0)
    {
        // log it
        throw new DivideByZeroException("Divide by zero attempted!");
    }

    return x / y; 
}
Mike Stone
I'm pretty sure he's talking about ASP.NET Session which doesn't behave that way.
Cade Roux
Yeah, I just don't have the docs open and I wanted to be clear that the assignment COULD be asynchronous, but more than likely isn't (and easy to tell if it isn't)
Mike Stone
FYI, with regards to throwing an exception in MyFunction... don't bother with performance of exceptions unless you are expecting an exception on every invoke in like a tight loop that is executed a lot... microperformance optimization like that makes the code unmaintainable
Mike Stone
From the OP's example, the spec seems to define dividing by zero to return 0 (hence, no need to throw the exception... otherwise you wouldn't need try/catch at all; just return x/y; and let the framework throw any exceptions)
SnOrfus
A: 

Quite strange. On the second item it may or may not be faster. It certainly isn't the same functionality though.

BlackWasp
A: 

I'm guessing your consultant is suggesting use a status variable instead of exception for error handling is a better practice? I don't agree. How often does people forgot or too lazy to do error checking for return values? Also, pass/fail variable is not informative. There are more things can go wrong other than divide by zero like integer x/y is too big or x is NaN. When things go wrong, status variable cannot tell you what went wrong, but exception can. Exception is for exceptional case, and divide by zero or NaN are definitely exceptional cases.

Alvin
+1  A: 

This guy does not know what he is doing. The obvious culprit is right here:

Session["UserID"] = SomeProprietarySessionManagementLookup();
MusiGenesis
Explain why? Seriously....I am curious.
Saif Khan
The consultant blamed ASP.NET for the swapped session information while inaccurately stating that session variables are assigned asynchronously. Far more likely that the proprietary session management code is flawed (in addition to being totaly superfluous).
MusiGenesis
Believe me, if ASP.NET's built-in session management system were prone to intermittently swapping user data, the uproar over this would have been tremendous.
MusiGenesis
A: 

The session thing is possible. It's a bug, beyond doubt, but it could be that the write arrives at whatever custom session state provider you're using after the next read. The session state provider API accommodates locking to prevent this sort of thing, but if the implementor has just ignored all that, your consultant could be telling the truth.

The second issue is also kinda valid. It's not quite idiomatic - it's a slightly reversed version of things like int.TryParse, which are there to avoid performance issues caused by throwing lots of exceptions. But unless you're calling that code an awful lot, it's unlikely it'll make a noticeable difference (compared to say, one less database query per page etc). It's certainly not something you should do by default.

Thom
+3  A: 

Asp.net sessions, if you're using the built-in providers, won't accidentally give you someone else's session. SomeProprietarySessionManagementLookup() is the likely culprit and is returning bad values or just not working.

Session["UserID"] = SomeProprietarySessionManagementLookup();

First of all assigning the return value from an asynchronously SomeProprietarySessionManagementLookup() just wont work. The consultants code probably looks like:

public void SomeProprietarySessionManagementLookup()
{
    // do some async lookup
    Action<object> d = delegate(object val)
    {
        LookupSession(); // long running thing that looks up the user.
        Session["UserID"] = 1234; // Setting session manually
    };

    d.BeginInvoke(null,null,null);               
}

The consultant isn't totally full of BS, but they have written some buggy code. Response.Redirect() does throw a ThreadAbort, and if the proprietary method is asynchronous, asp.net doesn't know to wait for the asynchronous method to write back to the session before asp.net itself saves the session. This is probably why it sometimes works and sometimes doesn't.

Their code might work if the asp.net session is in-process, but a state server or db server wouldn't. It's timing dependent.

I tested the following. We use state server in development. This code works because the session is written to before the main thread finishes.

Action<object> d = delegate(object val)
{
    System.Threading.Thread.Sleep(1000);  // waits a little
    Session["rubbish"] = DateTime.Now;
};

d.BeginInvoke(null, null, null);
System.Threading.Thread.Sleep(5000);      // waits a lot

object stuff = Session["rubbish"];
if( stuff == null ) stuff = "not there";
divStuff.InnerHtml = Convert.ToString(stuff);

This next snippet of code doesn't work because the session was already saved back to state server by the time the asynchronous method gets around to setting a session value.

Action<object> d = delegate(object val)
{
    System.Threading.Thread.Sleep(5000);  // waits a lot
    Session["rubbish"] = DateTime.Now;
};

d.BeginInvoke(null, null, null);

// wait removed - ends immediately.
object stuff = Session["rubbish"];
if( stuff == null ) stuff = "not there";
divStuff.InnerHtml = Convert.ToString(stuff);

The first step is for the consultant to make their code synchronous because their performance trick didn't work at all. If that fixes it, have the consultant properly implement using the Asynchronous Programming Design Pattern

Robert Paulson
I agree with you on this"Asp.net sessions, if you're using the built-in providers, won't accidentally give you someone else's session. SomeProprietarySessionManagementLookup() is the likely culprit and is returning bad values or just not working"
Saif Khan
I know this question is really old but I imagine you're giving that developer/team wayyyyy too much credit.
Chris Marisic
A: 

If SomeProprietarySessionManagementLookup(); is doing an asynchronous assignment it would more likely look like this:

SomeProprietarySessionManagementLookup(Session["UserID"]);

The very fact that the code is assigning the result to Session["UserID"] would suggest that it is not supposed to be asynchronous and the result should be obtained before Response.Redirect is called. If SomeProprietarySessionManagementLookup is returning before its result is calculated they have a design flaw anyway.

The throw an exception or use an out parameter is a matter of opinion and circumstance and in actual practice won't amount to a hill of beans which ever way you do it. For the performance hit of exceptions to become an issue you would need to be calling the function a huge number of times which would probably be a problem in itself.

Martin Brown
A: 

If the consultants deployed their ASP.NET application on your server(s), then they may have deployed it in uncompiled form, which means there would be a bunch of *.cs files floating around that you could look at.

If all you can find is compiled .NET assemblies (DLLs and EXEs) of theirs, then you should still be able to decompile them into somewhat readable source code. I'll bet if you look through the code you'll find them using static variables in their proprietary lookup code. You'd then have something very concrete to show your bosses.

MusiGenesis
A: 

This entire answer stream is full of typical programmer attitudes. It reminds me of Joel's 'Things you should never do' article (rewrite from scratch.) We don't really know anything about the system, other than there's a bug, and some guy posted some code online. There are so many unknowns that it is ridiculous to say "This guy does not know what he is doing."

Shawn Simon
A: 

Rather than pile on the Consultant, you could just as easily pile on the person who procured their services. No consultant is perfect, nor is a hiring manager ... but at the end of the day the real direction you should be taking is very clear: instead of trying to find fault you should expend energy into working collaboratively to find solutions. No matter how skilled someone is at their roles and responsibilities they will certainly have deficiencies. If you determine there is a pattern of incompentencies then you may choose to transition to another resource going forward, but assigning blame has never solved a single problem in history.

Nissan Fan