The following code hasbeen provided to me by a partner company for use in our asp.netapplication. Here is the class that I am questioning:
public class CurrentPageStyle
{
/// <summary>
/// The PageStyleRow for the current request.
/// </summary>
private static PageStylesDefinition.PageStyleRow currentpagestylerow;
/// <param name="ctxt">This gets the current pagestyle</param>
public CurrentPageStyle(HttpContext ctxt)
{
string keyword = ctxt.Request.QueryString["DView"];
if (keyword != null)
{
currentpagestylerow = TKConfiguration.Page_Styles_Definition.FindAll(keyword);
}
else
{
PageStylesDefinition.PageStylesRow toppsr = TKConfiguration.Page_Styles_Definition.GetFirst();
currentpagestylerow = TKConfiguration.Page_Styles_Definition.FindAll(toppsr.DefaultKeyword);
}
}
/// <summary>
/// This initializes the static member
/// </summary>
static CurrentPageStyle()
{
currentpagestylerow = null;
}
/// <summary>
/// Access to the current page style row for the request.
/// </summary>
public static PageStylesDefinition.PageStyleRow CurrentPageStyleRow
{
get
{
return currentpagestylerow;
}
set
{
currentpagestylerow = value;
}
}
Here is the usage from the global.asax file:
protected void Application_AcquireRequestState(object sender, EventArgs e)
{
CurrentPageStyle.CurrentPageStyleRow = null;
string[] pagestyle = Request.QueryString.GetValues('DView');
if (pagestyle != null)
{
if (pagestyle.Length != 0)
{
PageStylesDefinition.PageStyleRow psr = TKConfiguration.Page_Styles_Definition.FindAll(pagestyle[0]);
CurrentPageStyle.CurrentPageStyleRow = psr;
}
}
}
Myconcern is that the Application_AcquireRequestState event handler iscalled for every web request and it is setting a staticCurrentPageStyleRow which is then used later in the application. Myunderstanding is that there should only be one staticCurrentPageStyleRow per application and there can be web requests onmultiple threads so all of the threads will be sharing the sameCurrentPageStyleRow. Since there can be a differentCurrentPageStyleRow for each request, I think this is not thread safeand user 1 could end up getting user 2's CurrentPageStyleRow. I wantto make sure I am correct about this before reporting the issue to thepartner company. Am I right or am I off base?
Thanks,
Eric
In my modest opinion you should move that to an HttpModule to capture the request, and yes do not use an static class member! Why can't be an instance like the class?
Nice finding Eric! However some more people here may want to comment! Don't take my word for it
Al,
I am in the process of moving this to an HttpModule. As I said, the code was supplied by a partner company. I am in the process of adapting it to meet our needs. That was when I ran across this bit of code that does not appear to be thread safe. I am the only experienced .net developer on staff here, so I wanted someone else review the code and confirm my concerns before I reported it as a bug.
Thanks,
Eric
