Different interpretations of the bug lead to different fixes of varying quality
I mentioned that different people might characterize the bug in different ways. That's interesting in its own right, because it means that different people will "fix" the bug in different ways. The quality of the fix depends strongly on the developer's understanding of the bug.
In the case under discussion, the problem was described to me as
being that the myService instance is declared as
static. Now that's not how I myself would describe the problem at all,
even though I understand the explanation. I wouldn't even describe the
problem as being that the myService instance
is shared, which is the most charitable reading of the
offered explanation.
If you believe that the problem is that myService is
shared, then you might "fix" the problem like so:
MyServlet.java, "fixed"
package demoapp;
import java.io.IOException;
import java.util.List;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
public class MyServlet extends HttpServlet {
public void doGet(HttpServletRequest req, HttpServletResponse res)
throws IOException, ServletException {
Long userId = (Long) req.getSession().getAttribute("userId");
MyService myService = new MyService();
List userData = myService.getUserData(userId);
req.setAttribute("userData", userData);
req.getRequestDispatcher("/WEB-INF/jsp/list.jsp").forward(req, res);
}
}
And in fact that's exactly how the problem was "fixed".
I think that if a senior developer looks at the code above, he will immediately see a red flag. Services are often defined as stateless, and so before accepting this fix, he'd want to know whether this is (or should be) a stateless service, because if it is, it would be strange to create a new service instances on a per-request basis. And he'd also want to know how expensive the construction of the service was before creating one with every request. Following that line of thought leads one to the real issue, which is that the DAO is unnecessarily stateful, and its statefulness creates a threadsafety problem that leaks into the service and ultimately the servlet. The real fix is to do the following:
MyDao.java, actually fixed
package demoapp;
import java.util.List;
public class MyDao {
public List readUserData(Long userId) {
List myData =
...do some JDBC, process the result set, get the data...
return myData;
}
}
Even though the two fixes both solve the issue at hand, only the second one solves the issue "permanently". It's a lot harder for the client to misuse the DAO with the second approach; the first approach does nothing to prevent new instances of misuse.
Comments (16)
Post a comment
