1

I have an asp repeater which sets a generichandler (.ashx) parameter differently for each entry. This is used to create comments that are posted by users and display the avatar of the user posting by fetching it from a database. If there is only one post, it works fine, but as soon as there are two or more posts I get a crash in the "GetIdFromUserName" method on the line "connection.Open()" (code provided below) saying "The connection was not closed, the connections current state is open". Bearing in mind this only happens when more than one comment exists, I have no idea what causes this.

One thing I have found out, though: if I make the asp:image object not use the "Eval" (so it only shows the same image for every post regardless of ) it works, but beyond that I have no idea. Eval does work fine if there's only one post to show, though, like mentioned earlier.

If anyone have any idea how to fix this I'd be super grateful.

ASP code:

        <asp:Repeater ID="CommentsRepeater" runat="server" OnItemDataBound="CommentsRepeater_ItemDataBound">
            <ItemTemplate>
                <div class="comment">
                    <div class="commentInfo">
                        <strong>
                            <asp:Image ID="Avatar" ImageUrl='<%# "CommentsAvatarHandler.ashx?id=" + Eval("UserName") %>' runat="server" Height="100px" Width="100" />
                            <asp:LinkButton ID="CommentName" Text='<%# Eval("UserName") %>' CommandArgument='<%# Eval("UserId") %>' CausesValidation="false" runat="server"></asp:LinkButton></strong> @ 
                        <asp:Label ID="CommentDate" Text='<%# Eval("PostedDate") %>' runat="server"></asp:Label>
                        <div class="commentVote">
                            <asp:LinkButton ID="VoteUp" CommandArgument='<%# Eval("CommentId") %>' OnClick="VoteUpClick" CausesValidation="false" runat="server"><img src="images/pluss2.png" /></asp:LinkButton>
                            <asp:LinkButton ID="VoteDown" CommandArgument='<%# Eval("CommentId") %>' OnClick="VoteDownClick" CausesValidation="false" runat="server"><img src="images/minus2.png" /></asp:LinkButton>
                            <strong>(<asp:Label ID="CommentVote" Text='<%# Eval("Vote") %>' runat="server"></asp:Label>)</strong>
                        </div>
                    </div>
                    <div class="commentText">
                        <asp:Label ID="CommentText" Text='<%# Eval("Comment") %>' runat="server"></asp:Label>
                    </div>
                </div>
            </ItemTemplate>
            <SeparatorTemplate>
                <br />
            </SeparatorTemplate>
        </asp:Repeater>

Relevant bit:

   <asp:Image ID="Avatar" ImageUrl='<%# "CommentsAvatarHandler.ashx?id=" + Eval("UserName") %>' runat="server" Height="100px" Width="100" />

Here's the handler:

public class CommentsAvatarHandler : IHttpHandler
{

    public void ProcessRequest(HttpContext context)
    {
        String userName = context.Request.QueryString["id"];
        Gamez.Sql.GetAvatarOfUser(context, userName);
    }

    public bool IsReusable
    {
        get
        {
            return false;
        }
    }
}

the method used by the handler:

    public static void GetAvatarOfUser(HttpContext context, String userName)
    {
        SqlDataReader reader;

        try
        {
            Guid id = GetIdFromUserName(userName);
            command.CommandText = "SELECT Avatar FROM UserAttributes WHERE UserId = @id";
            command.Parameters.AddWithValue("@id", id);

            connection.Open();
            reader = command.ExecuteReader();
            while (reader.Read())
            {
                try
                {
                    context.Response.ContentType = "image/jpg";
                    context.Response.BinaryWrite((byte[])reader["Avatar"]);
                }
                catch { }
            }
            if (reader != null)
                reader.Close();

        }
        finally
        {
            if (connection != null)
            {
                connection.Close();
            }
        }

    }

The GetIdFromUserName method:

    public static Guid GetIdFromUserName(String name)
    {
        command.Parameters.Clear();
        command.CommandText = "SELECT UserId FROM aspnet_Users WHERE UserName = @UserName";
        command.Parameters.AddWithValue("@UserName", name);
        connection.Open();  // <---- DESCRIBED CRASH HAPPENS HERE
        Guid id = (Guid)command.ExecuteScalar();
        connection.Close();
        return id;
    }

GetAvatarFromUser and GetIdFromUserName are in the same library and have access to the same SQLCommand (command) and SQLConnection (connection) objects.

user1339253
  • 149
  • 1
  • 2
  • 15

1 Answers1

2

From your code it looks like you are using the same instance of connection and command for each request. I am assuming both of these are page level variables?

Create a separate instance of both of these variable for each sql request.

Additionally, you should wrap your Connection and Command initializations in a using block. This will take care of closing and disposing of both resources correctly and using best practices. Something like this:

using (SqlConnection conn = [CONNECTION INITIALIZATION])
   {
   using (SqlCommand command = new SqlCommand(conn))
      {
      ...
         using (SqlDataReader reader = command.ExecuteReader())
         {
            .....
         }
      }
   }

Currently, the way you have it set up, you are asking for trouble.

EDIT

Your code fails when there is a second post because you are sharing your connection object across multiple requests. So while the first post's information is retrieved (opening the connection), you are attempting to retrieve another post (again, trying to open the same connection).

Does that make sense?

Shai Cohen
  • 6,074
  • 4
  • 31
  • 54
  • Yes, this does indeed work. Thanks. I'm still confused why it hiccups, though, as I've made absolutely sure to close it after every time I open it. If I have to initialize the connection and command in every single request, it will lead to a lot of redundant code =/ Is there any way to get around this? In addition, what benefit does using "using" have (it works without it)? Thanks – user1339253 Apr 24 '13 at 19:49
  • The using block is effectively a try finally block that calls an object's dispose method. The way your code is structured now, if this line fails `Guid id = (Guid)command.ExecuteScalar();`, the connection will never be closed. By wrapping it in a using block, it guarantees that the connections Dispose method is always called. Also, if this answer helped you solved your issue, please accept it as an answer. Thanks! – Shai Cohen Apr 24 '13 at 21:07
  • I see. But command uses connection as it's SQLConnection (==> `static SqlCommand command = new SqlCommand("", connection);`), and I have a `connection.Close();` line after the line you mentioned, so why would it never be closed? – user1339253 Apr 24 '13 at 21:23
  • If your SQL errors out on that line, an exception is thrown. When an exception is thrown, no other lines of code are processed. The code jumps straight to the next `catch` or `finally` block. Actually in your case, you would get away with it, because the call to this method ***is*** wrapped in a try/finally. But imagine a few months down the line you want to reuse this method and you forget to wrap that call. It's just a matter of best practices. – Shai Cohen Apr 24 '13 at 21:29
  • I see what you mean by best practices, so basically always make sure to use try/finally or using. But best practices aside, as you said technically my code shouldn't have this issue, which is why I'm interested in why it's not working. Thanks for your help, I accepted it as the answer. – user1339253 Apr 24 '13 at 21:39
  • Thank you! Just to be clear, the reason your code was not working doesn't have to do with the try/finally (or using) block. It has to do with the fact that you were sharing your connection across different requests. – Shai Cohen Apr 24 '13 at 21:50