javamavendebugginggarbage-collectionapache-commons-dbcp

Is it ok to use same BasicDataSource, Connection, Statement and ResultSet Object in multiple class methods.?


I have below code which uses static objects of BasicDataSource, Sql Connection, Statement and ResultSet. The code below is working fine, but i just want to know about the safety of using these kinds of coding practices. or how can i optimize the below code so that it can become more stable and can reliable.

public class Testing {
     static BasicDataSource bds = DBConnection.getInstance().getBds();
     static Connection con = null;
     static PreparedStatement stmt = null;
     static ResultSet rs = null;

    private void show() {
        try {
            con = bds.getConnection();
            stmt = con.prepareStatement("SELECT * FROM users");
            rs = stmt.executeQuery();
            if(rs.next()) {
                System.out.println(rs.getString("firstname") + " " + rs.getString("lastname"));
            }
        } catch (SQLException e) {
            e.printStackTrace();
        }finally {
            try {
                con.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }
    }
    private void display() {
        try {
            con = bds.getConnection();
            stmt = con.prepareStatement("SELECT * FROM agent_cities");
            rs = stmt.executeQuery();
            while(rs.next()) {
                System.out.println(rs.getString("city_name"));
            }
        } catch (SQLException e) {
            e.printStackTrace();
        }finally {
            try {
                con.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }
    }
    private void add() {
        try {
            con = bds.getConnection();
            stmt = con.prepareStatement("UPDATE users SET firstname = 'shsh' WHERE id = 2");
            stmt.executeUpdate();
            System.out.println("updated successfully");
        } catch (SQLException e) {
            e.printStackTrace();
        }finally {
            try {
                con.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }
    }
    public static void main(String[] args) {
        Testing t = new Testing();
        t.show();
        t.display();
        t.add();
    }
}

Thanks in advance. Do share your cases on which you can break above code and question about its safety.

Update : Updating only to ensure that no one should use static fields as i have used in above program because above program contains bug when deployed on dev server.

After using above code on large systems i found the bug. one month ago i had no problem with the above code and it was working fine but today i found the bug.

Bug:

After hitting my APIs 6-7 times it stopped giving response at 8th hit. i really don't know why and have no idea about loop holes present in program. But now as i have accepted the answer i changed my source code and started using try-with resources in my code and removed static fields.

But i am still curious to know about the bug that i found in the above code. that doesn't gives response and hangs after 7-8 API hits. Please share your thoughts on this. i'm using apache tomcat 8.5.32 server. Thanks in advance.


Solution

  • Better use try-with-resources. This automatically closes Connection, Statement and ResultSet, even when an exception was raised, or on an inner return.

        String sql = "UPDATE users SET firstname = ? WHERE id = ?";
        try (Connection con = bds.getConnection();
                PreparedStatement stmt = con.prepareStatement()) {
            stmt.setString(1, "shsh");
            stmt.setLong(2, 2);
            stmt.executeUpdate();
            System.out.println("updated successfully");
        }
    
        String sql = "SELECT city_name FROM agent_cities";
        try (Connection con = bds.getConnection();
                PreparedStatement stmt = con.prepareStatement()) {
            try (ResultSet rs = stmt.executeQuery()) {
                while(rs.next()) {
                    System.out.println(rs.getString("city_name"));
                }
            }
        }
    

    This is better for garbage collection. Prevents unnice rs2, rs3. Allows multi-user concurrency, like in a server application. Calls that query themselves. And static is even more in the style of global variables.