to open and close or no need to

  • Thread starter Thread starter JohnE
  • Start date Start date
J

JohnE

Hello. I am working on an ASP.net webapplication (using C#) and I have a
gridview that fills with data when it opens. Below is the code that fills
the gridview. Now, what I am getting is conflicting information from
co-workers that I need to open the connection and then close it. To me, for
this gridview there is no need to open and close the connection since I'm
filling an underlying dataview. There is a Refresh button available that
restores the gridview back to its original settings but since it is true, the
refresh does not enter into the if statement of the page_load.

private DataView bindgrid()
{
string connStr =
ConfigurationManager.ConnectionStrings["ProteusConnectionString"].ConnectionString;
string sql = "SEL_FillChangeRequestListGridView";
SqlDataAdapter sqlDa = new SqlDataAdapter(sql, connStr);
DataSet ds = new DataSet();
sqlDa.Fill(ds);
DataView dv = new DataView();

if (ViewState["sortExpr"] != null)
{
dv = new DataView(ds.Tables[0]);
dv.Sort = (string)ViewState["sortExpr"] + " " +
ViewState["sortDir"];
}
else
dv = ds.Tables[0].DefaultView;

return dv;
}

protected void Page_Load(object sender, EventArgs e)
{
try
{
if (!Page.IsPostBack)
{
DataView dv = new DataView();
dv = bindgrid();
gvwChangeRequestList.DataSource = dv;
gvwChangeRequestList.DataBind();
}
lblSortColumn.Visible = true;
lblSortColumn.Text = "Default sort: STATUS, workflow in
descending order.";
PageCount();
TotalCount();
}

catch
{
string message = "The connection failed on the page load. Try
again or contact Administrator.";
System.Text.StringBuilder sb = new System.Text.StringBuilder();
sb.Append("<script type='text/javascript'>");
sb.Append("window.onload=function(){");
sb.Append("alert('");
sb.Append(message);
sb.Append("')};");
sb.Append("</script>");
ClientScript.RegisterClientScriptBlock(GetType(), "alert",
sb.ToString());
}
}

So, I guess my question is, do I need to open and close the connection on
this? Also, what is the "standard" (or rule of thumb) for when connection is
opened and closed?

Thanks.
John
 
To be clear here, you are not filling a DataView. A DataView is just
that... a view of some data. You are filling a DataSet with data from your
database. Also, take the DataGrid out of the equation, as it is nothing
more than a way to display the data in the DataSet that you've gotten.

So, the question really is, should your connection be closed after getting
your data and putting it into a DataSet?

The short answer is: absolutley, yes!

ADO .NET is primarially about working with disconnected data and the DataSet
object was created for using data in this way.

Now, having given that little bit of lecture, I said that your connection
should be closed after you've gotten your data, but that doesn't mean you
need to explicitly close it. A DataAdapter will always leave the connection
just the way it found it. If it was closed when you called .Fill, the
DataAdapter will open the connection to do its work, but then close the
connection once the work is done. So, if the connection is closed to begin
with, it will wind up being closed at the end.

Now, to add just a little more complexity here, your code does not use
try...catch exception handling, which it absolutely should be doing. As
such, it is very common to put an explicit connection close statement in the
finally section of the try...catch, in case your initial code bombs out
before the DataAdapter has a chance to close the connection for you.

-Scott


JohnE said:
Hello. I am working on an ASP.net webapplication (using C#) and I have a
gridview that fills with data when it opens. Below is the code that fills
the gridview. Now, what I am getting is conflicting information from
co-workers that I need to open the connection and then close it. To me,
for
this gridview there is no need to open and close the connection since I'm
filling an underlying dataview. There is a Refresh button available that
restores the gridview back to its original settings but since it is true,
the
refresh does not enter into the if statement of the page_load.

private DataView bindgrid()
{
string connStr =
ConfigurationManager.ConnectionStrings["ProteusConnectionString"].ConnectionString;
string sql = "SEL_FillChangeRequestListGridView";
SqlDataAdapter sqlDa = new SqlDataAdapter(sql, connStr);
DataSet ds = new DataSet();
sqlDa.Fill(ds);
DataView dv = new DataView();

if (ViewState["sortExpr"] != null)
{
dv = new DataView(ds.Tables[0]);
dv.Sort = (string)ViewState["sortExpr"] + " " +
ViewState["sortDir"];
}
else
dv = ds.Tables[0].DefaultView;

return dv;
}

protected void Page_Load(object sender, EventArgs e)
{
try
{
if (!Page.IsPostBack)
{
DataView dv = new DataView();
dv = bindgrid();
gvwChangeRequestList.DataSource = dv;
gvwChangeRequestList.DataBind();
}
lblSortColumn.Visible = true;
lblSortColumn.Text = "Default sort: STATUS, workflow in
descending order.";
PageCount();
TotalCount();
}

catch
{
string message = "The connection failed on the page load. Try
again or contact Administrator.";
System.Text.StringBuilder sb = new System.Text.StringBuilder();
sb.Append("<script type='text/javascript'>");
sb.Append("window.onload=function(){");
sb.Append("alert('");
sb.Append(message);
sb.Append("')};");
sb.Append("</script>");
ClientScript.RegisterClientScriptBlock(GetType(), "alert",
sb.ToString());
}
}

So, I guess my question is, do I need to open and close the connection on
this? Also, what is the "standard" (or rule of thumb) for when connection
is
opened and closed?

Thanks.
John
Click to expand...
 
The rule of thumb is "little and often". I.e. open the connection
every time it's required and then close it as soon as it's no longer
required. This is especially true for web apps.
Click to expand...

To add to Mark's post. The reason to close is that you do not really
have to reopen the connection itself, as the true connection object is
cached by ADO.NET in a pool. If you pull information from the database
numerous times, you are not creating and destroying the connection each
time, as the connection is released to the pool.

The end result is it only takes a millisecond to grab the connection
again, unless you have been idle, without connections, for a long time.
From your code standpoint, it may appear you are opening and closing
often.

Now, one more "rule of thumb" is when you open a connection, go ahead
and get as much data as you need. Don't run a lot of petty little CRUD
statements, grabbing only one item and then releasing to pool. Think in
terms of what the page needs.

Beyond that, don't try to use a single connection for an application,
unless you have some need to maintain an open connection (rare in
windows, non-existent (or nearly so) in web apps).

Peace and Grace,

--
Gregory A. Beamer (MVP)

Twitter: @gbworld
Blog: http://gregorybeamer.spaces.live.com

*******************************************
| Think outside the box! |
*******************************************
 
using (SqlDataAdapter sqlDa = new SqlDataAdapter(sql, connStr))
{
using (DataSet ds = new DataSet())
{
// rest of the code goes here
}
}
Click to expand...

Just remember here you have to consume (bind) the data prior to the end of
the using for the DataSet if you use this pattern. ;-)

Peace and Grace,

--
Gregory A. Beamer (MVP)

Twitter: @gbworld
Blog: http://gregorybeamer.spaces.live.com

*******************************************
| Think outside the box! |
*******************************************
 
Code Review


BAD NAME - you are puling hte data view here and not actually binding, so
this is a bad naming convention.
private DataView bindgrid()
{
string connStr =
ConfigurationManager.ConnectionStrings["ProteusConnectionString"].Conne
ctionString;
string sql = "SEL_FillChangeRequestListGridView";
Click to expand...

This is an implicit method of setting up the data retrieval, leaving you no
control over the objects used by the DataAdapter. I suggest a pattern more
like:


private DataView GetData(string sortExpression, string sortDirection)
{
string sql = "your statement";
string connString = ConfigurationManager
.ConnectionStrings["{yours}"].ConnectionString;

SqlConnection conn = new SqlConnection(connString);
SqlCommand command = new SqlCommand(sqlStatement, conn);
DataSet ds = new DataSet();
SqlDataAdapter da = new SqlDataAdapter(command);
//Optional table mappings here

try
{
conn.Open;
da.Fill(ds);
}
finally
{
conn.Dispose();

}

table ds.Tables[0];

if(sortExpression != null)
{
DataView view = new DataView();
view.Sort = sortExpression + " " + sortDirection;
}

return table.DefaultView;
}

This allows you to open and dispose as quickly as possible. This can be
improved even more, but it follows your pattern as closely as possible so
you can replace the routine.

SqlDataAdapter sqlDa = new SqlDataAdapter(sql, connStr);
DataSet ds = new DataSet();
sqlDa.Fill(ds);
DataView dv = new DataView();

if (ViewState["sortExpr"] != null)
{
dv = new DataView(ds.Tables[0]);
dv.Sort = (string)ViewState["sortExpr"] + " " +
ViewState["sortDir"];
}
else
dv = ds.Tables[0].DefaultView;

return dv;
}
Click to expand...

It is also better, to separate out the binding into its own routine, so you
can alter the bind with postbacks.
protected void Page_Load(object sender, EventArgs e)
{
Click to expand...

Not fond of try ... catch at this level. Better to handle the error when
you bind, which should be in its own routine.
try
{
if (!Page.IsPostBack)
{
DataView dv = new DataView();
dv = bindgrid();
gvwChangeRequestList.DataSource = dv;
gvwChangeRequestList.DataBind();
}
lblSortColumn.Visible = true;
lblSortColumn.Text = "Default sort: STATUS, workflow in
descending order.";
PageCount();
TotalCount();
}

catch
{
string message = "The connection failed on the page load.
Try
again or contact Administrator.";
System.Text.StringBuilder sb = new
System.Text.StringBuilder(); sb.Append("<script
type='text/javascript'>");
sb.Append("window.onload=function(){");
sb.Append("alert('"); sb.Append(message);
sb.Append("')};");
sb.Append("</script>");
ClientScript.RegisterClientScriptBlock(GetType(), "alert",
sb.ToString());
}
}
Click to expand...

Hope this helps.

Peace and Grace,

--
Gregory A. Beamer (MVP)

Twitter: @gbworld
Blog: http://gregorybeamer.spaces.live.com

*******************************************
| Think outside the box! |
*******************************************
 
Can you not just get as much data as you need? Do you actually have to
"go ahead and" get as much data as you need...?

;-)
Click to expand...

I guess you could go sideways too. :P

LOL

Peace and Grace,

--
Gregory A. Beamer (MVP)

Twitter: @gbworld
Blog: http://gregorybeamer.spaces.live.com

*******************************************
| Think outside the box! |
*******************************************
 
To add to Gregory's and Mark's post:
If you open and close just in time then there is another advantage - it is
thread safe (as long as you don't pass connection object to another thread).
There is really no arguments for keeping a single connection open.

So, go ahead :-P and do use open right before and close asap pattern.
 
Indeed. I never understood why you Americans always have to "go ahead
and" do everything... ;-)
Click to expand...

Because we have a bigger country. :P

Peace and Grace,

--
Gregory A. Beamer (MVP)

Twitter: @gbworld
Blog: http://gregorybeamer.spaces.live.com

*******************************************
| Think outside the box! |
*******************************************
 
All good suggestions... but to be 100% clear: If you execute the Open method
on a Connection object you must ensure that the code executes a Close method
as well--even if an exception is fired. Unlike older (pre-.NET) runtimes,
you cannot depend on the .NET Garbage Collector (GC) to close the connection
(release it to the connection pool) in a timely fashion. Leaving Connection
objects unclosed is a very common practice and leads to the connection pool
overflowing and your application to crash with timeout exceptions. Note that
the Fill method opens (and closes) the associated connection ONLY if it was
closed to begin with. It's a great idea to monitor the connection pool
associated with your application domain to ensure that the pool is not
leaking...

hth

--
__________________________________________________________________________
William R. Vaughn
President and Founder Beta V Corporation
Author, Mentor, Dad, Grandpa
Microsoft MVP
(425) 556-9205 (Pacific time)
Hitchhiker's Guide to Visual Studio and SQL Server (7th Edition)
http://betav.com http://betav.com/blog/billva
____________________________________________________________________________________________



Gregory A. Beamer said:
Code Review


BAD NAME - you are puling hte data view here and not actually binding, so
this is a bad naming convention.
private DataView bindgrid()
{
string connStr =
ConfigurationManager.ConnectionStrings["ProteusConnectionString"].Conne
ctionString;
string sql = "SEL_FillChangeRequestListGridView";
Click to expand...

This is an implicit method of setting up the data retrieval, leaving you
no
control over the objects used by the DataAdapter. I suggest a pattern more
like:


private DataView GetData(string sortExpression, string sortDirection)
{
string sql = "your statement";
string connString = ConfigurationManager
.ConnectionStrings["{yours}"].ConnectionString;

SqlConnection conn = new SqlConnection(connString);
SqlCommand command = new SqlCommand(sqlStatement, conn);
DataSet ds = new DataSet();
SqlDataAdapter da = new SqlDataAdapter(command);
//Optional table mappings here

try
{
conn.Open;
da.Fill(ds);
}
finally
{
conn.Dispose();

}

table ds.Tables[0];

if(sortExpression != null)
{
DataView view = new DataView();
view.Sort = sortExpression + " " + sortDirection;
}

return table.DefaultView;
}

This allows you to open and dispose as quickly as possible. This can be
improved even more, but it follows your pattern as closely as possible so
you can replace the routine.

SqlDataAdapter sqlDa = new SqlDataAdapter(sql, connStr);
DataSet ds = new DataSet();
sqlDa.Fill(ds);
DataView dv = new DataView();

if (ViewState["sortExpr"] != null)
{
dv = new DataView(ds.Tables[0]);
dv.Sort = (string)ViewState["sortExpr"] + " " +
ViewState["sortDir"];
}
else
dv = ds.Tables[0].DefaultView;

return dv;
}
Click to expand...

It is also better, to separate out the binding into its own routine, so
you
can alter the bind with postbacks.
protected void Page_Load(object sender, EventArgs e)
{
Click to expand...

Not fond of try ... catch at this level. Better to handle the error when
you bind, which should be in its own routine.
try
{
if (!Page.IsPostBack)
{
DataView dv = new DataView();
dv = bindgrid();
gvwChangeRequestList.DataSource = dv;
gvwChangeRequestList.DataBind();
}
lblSortColumn.Visible = true;
lblSortColumn.Text = "Default sort: STATUS, workflow in
descending order.";
PageCount();
TotalCount();
}

catch
{
string message = "The connection failed on the page load.
Try
again or contact Administrator.";
System.Text.StringBuilder sb = new
System.Text.StringBuilder(); sb.Append("<script
type='text/javascript'>");
sb.Append("window.onload=function(){");
sb.Append("alert('"); sb.Append(message);
sb.Append("')};");
sb.Append("</script>");
ClientScript.RegisterClientScriptBlock(GetType(), "alert",
sb.ToString());
}
}
Click to expand...

Hope this helps.

Peace and Grace,

--
Gregory A. Beamer (MVP)

Twitter: @gbworld
Blog: http://gregorybeamer.spaces.live.com

*******************************************
| Think outside the box! |
*******************************************
Click to expand...
 
All good suggestions... but to be 100% clear: If you execute the Open
method on a Connection object you must ensure that the code executes a
Close method as well--even if an exception is fired. Unlike older
(pre-.NET) runtimes, you cannot depend on the .NET Garbage Collector
(GC) to close the connection (release it to the connection pool) in a
timely fashion. Leaving Connection objects unclosed is a very common
practice and leads to the connection pool overflowing and your
application to crash with timeout exceptions. Note that the Fill
method opens (and closes) the associated connection ONLY if it was
closed to begin with. It's a great idea to monitor the connection pool
associated with your application domain to ensure that the pool is not
leaking...
Click to expand...

Adding to this:

This leads us to either:

try
{
connection.Open();
}
finally
{
connection.Dispose();
}

or

using(SqlConnection connection = new SqlConnection(connString))
{
}

which is essentially the same thing (check the IL in Reflector). There
are still exceptions that can get around this, but if you are throwing
out of memory exceptions, you are in trouble anyway.




--
Gregory A. Beamer (MVP)

Twitter: @gbworld
Blog: http://gregorybeamer.spaces.live.com

*******************************************
| Think outside the box! |
*******************************************
 
I favour the using (...) structure, but that's nothing more than a
personal preference - the try{...}catch{...}finally{...} structure is
functionally identical...
Click to expand...

And almost 100% identical in IL:
http://bit.ly/741MOL

Peace and Grace,

--
Gregory A. Beamer (MVP)

Twitter: @gbworld
Blog: http://gregorybeamer.spaces.live.com

*******************************************
| Think outside the box! |
*******************************************
 
Back
Top