Правильный способ обработки соединений с базой данных в веб-приложении среднего размера - PullRequest
4 голосов
/ 09 июля 2011

В настоящее время я поддерживаю небольшое и среднее по размеру java-приложение (использующее только простые JSP / сервлеты), которое практикант сделал для внутреннего использования в компании, и у меня возникли некоторые проблемы с соединениями.

Иногда просто из ниоткуда мы получаем ошибки типа «Заявление закрыто» или «Соединение закрыто», и тогда все приложение просто перестает работать, и сервер должен быть перезапущен.

У меня нет большого опыта, и у меня нет никого, кто бы наставлял или учил меня относительно лучших практик, шаблонов проектирования и т. Д., Но я уверен, что это неправильный способ сделать это. Я читал о таких вещах, как DAL, DAO и DTO. Наше приложение не имеет ни одного из них.

Целое веб-приложение (т. Е. Сервлеты) в основном заполнено вызовами, подобными следующим:

Database db = Database.getInstance();
db.execute("INSERT INTO SomeTable VALUES (a, b, c)");
db.execute("UPDATE SomeTable SET Col = Val");

ВЫБОРЫ сделаны так:

ArrayList<Model> results = Model.fetch("SELECT * FROM SomeTable");

Где Model - это класс, расширяющий HashMap и представляющий одну строку в таблице.

Это код для Database.java, и мне было интересно, может ли кто-нибудь указать на очевидные вещи, которые не правы (я уверен, что их много), какие-нибудь быстрые исправления, которые можно сделать, и некоторые ресурсы о лучших практиках с С уважением к соединениям с базой данных / обработки соединений.

package classes;

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.ResultSetMetaData;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.ArrayList;
import java.util.HashMap;

import javax.naming.InitialContext;
import javax.naming.NamingException;
import javax.sql.DataSource;

public final class Database {

    public static Database getInstance() {
        if (Database.instance == null) {
            Database.instance = new Database();
        }
        return Database.instance;
    }

    // Returns the results for an SQL SELECT query.
    public ArrayList<HashMap<String, Object>> fetch(String sql) {

        ArrayList<HashMap<String, Object>> results = new ArrayList<HashMap<String, Object>>();

        try {

            PreparedStatement stmt = this.connection.prepareStatement(sql, ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY, ResultSet.HOLD_CURSORS_OVER_COMMIT);
            ResultSet rs = stmt.executeQuery();
            this.doFetch(rs, results);
            stmt.close();

        } catch (SQLException e) {
            this.handleException(e, sql);
        }

        return results;
    }

    public ArrayList<HashMap<String, Object>> fetch(String sql, ArrayList<Object> parameters) {

        ArrayList<HashMap<String, Object>> results = new ArrayList<HashMap<String, Object>>();

        try {

            // Bind parameters to statement.
            PreparedStatement pstmt = this.connection.prepareStatement(sql, ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY, ResultSet.HOLD_CURSORS_OVER_COMMIT);
            for (int i=0; i<parameters.size(); i++) {
                pstmt.setObject(i+1, parameters.get(i));
            }

            ResultSet rs = pstmt.executeQuery();
            this.doFetch(rs, results);
            pstmt.close();

        } catch (SQLException e) {
            this.handleException(e, sql, parameters);
        }

        return results;
    }

    public int execute(String sql) {
        int result = 0;
        try {
            Statement stmt = this.connection.createStatement();
            result = stmt.executeUpdate(sql);
            stmt.close();
        } catch (SQLException e) {
            this.handleException(e, sql);
        }
        return result;
    }

    public int execute(String sql, ArrayList<Object> parameters) {
        int result = 0;
        try {
            PreparedStatement pstmt = this.connection.prepareStatement(sql, ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY, ResultSet.HOLD_CURSORS_OVER_COMMIT);
            for (int i=0; i<parameters.size(); i++) {
                if (parameters.get(i) == null) {
                    pstmt.setNull(i+1, java.sql.Types.INTEGER);
                } else {
                    pstmt.setObject(i+1, parameters.get(i));
                }
            }
            result = pstmt.executeUpdate();
            pstmt.close();
        } catch (SQLException e) {
            this.handleException(e, sql, parameters);
        }
        return result;
    }

    public void commit() {
        try {
            this.connection.commit();
        } catch (SQLException e) {
            System.out.println("Failed to commit transaction.");
        }
    }

    public Connection getConnection() {
        return this.connection;
    }


    private static Database instance;
    private static DataSource dataSource = null;
    private Connection connection;

    private Database() {
        this.connect();
        this.execute("SET SCHEMA " + Constant.DBSCHEMA);
    }

    private void connect() {
        Connection connection = null;
        if (dataSource == null) {
            try {
                InitialContext initialContext = new InitialContext();
                dataSource = (DataSource)initialContext.lookup(
                        Constant.DEPLOYED ? Constant.PROD_JNDINAME : Constant.TEST_JNDINAME);
            } catch (NamingException e) {
                e.printStackTrace();
            }
        }
        try {
            connection = dataSource.getConnection();
        } catch (SQLException e) {
            e.printStackTrace();
        }
        this.connection = connection;
    }

    // Fetches the results from the ResultSet into the given ArrayList.

    private void doFetch(ResultSet rs, ArrayList<HashMap<String, Object>> results) throws SQLException {
        ResultSetMetaData rsmd = rs.getMetaData();

        ArrayList<String> cols = new ArrayList<String>();           
        int numCols = rsmd.getColumnCount();

        for (int i=1; i<=numCols; i++) {
            cols.add(rsmd.getColumnName(i));
        }

        while (rs.next()) {
            HashMap<String, Object> result = new HashMap<String, Object>();
            for (int i=1; i<=numCols; i++) {
                result.put(cols.get(i-1), rs.getObject(i));
            }
            results.add(result);
        }

        rs.close();
    }

    private void handleException(SQLException e, String sql) {
        System.out.println("SQLException " + e.getErrorCode() + ": " + e.getMessage());
        System.out.println("Statement: " + sql);
        ExceptionAdapter ea = new ExceptionAdapter(e);
        ea.setSQLInfo(e, sql);
        throw ea;
    }

    private void handleException(SQLException e, String sql, ArrayList<Object> parameters) {
        if (parameters.size() < 100) {
            System.out.println("SQLException " + e.getErrorCode() + ": " + e.getMessage());
            System.out.println("PreparedStatement: " + sql.replace("?", "[?]"));
            System.out.println("Parameters: " + parameters.toString());
        }
        ExceptionAdapter ea = new ExceptionAdapter(e);
        ea.setSQLInfo(e, sql, parameters);
        throw ea;
    }
}

Спасибо!

Ответы [ 4 ]

13 голосов
/ 09 июля 2011

Класс никогда не закрывает соединение: this.connection.close(). Поскольку Database является Singleton , приложение не использует пул соединений (источник данных). Для всех входящих запросов используется только одно соединение.

Практическое правило: получите одно соединение на метод (может быть, на оператор SQL). dataSource.getConnection() не дорого.

Вот как я бы провел рефакторинг класса:

  1. удалите открытый метод getConnection, если он использовался вне класса Database, у вас действительно есть проблема проектирования
  2. удалить метод commit. Я полагаю, это не имеет смысла, поскольку connection.setAutoCommit(false) никогда не вызывается, и я не вижу rollback метода
  3. удалить переменную экземпляра connection, вместо этого получить соединение за вызов
  4. и правильно закройте это соединение в блоке finally каждого вызова

Отказ от ответственности: Понятия не имею, как работает ваша транзакция в данный момент, поэтому я могу ошибаться с # 2.

Пример кода для метода получения соединения:

Connection c = null;
try {
    c = this.dataSource.getConnection();
    c.executeStatement("select * from dual");
} catch (SQLException e) {
    // handle...
} finally {
    closeConnection(c);
}

Интересно, как это приложение может работать вообще: -)

1 голос
/ 11 июня 2018

Чтобы ответить на ваш вопрос о принципах проектирования, этот объект по сути является объектом DAO, он просто не использует соглашение об именах, также в большом приложении может быть несколько таких объектов для разных типов данных (возможно, с использованием базового DAO). объект, от которого все они наследуются).

Общая идея заключается в том, что DAO - это центральное место, где обрабатываются соединения с базой данных, поэтому у вас нет всего этого кода в объекте Controller.

Помимо недостатков, уже отмеченных другими, это некий солидный код, написанный кем-то, кто очень хорошо понимает объектно-ориентированное программирование. Моя рекомендация состоит в том, чтобы изменить объект с одноэлементного и управлять подключением к базе данных с помощью пула соединений (как уже упоминалось другими). ​​

Похоже, что это абстрагированный объект, который возвращает массив карт (пар значений ключей), который можно использовать для разных типов данных, который затем используется в объектах модели или типах данных для создания объектов Java с возвращенной информацией.

1 голос
/ 09 июля 2011

Вы используете соединение JDBC очень небезопасным способом. Доступ к нему можно получить из нескольких потоков, и он не является потокобезопасным. Это веб-приложение, и несколько пользователей могут одновременно отправлять несколько запросов. Это маленькое чудо, ваше приложение не падает чаще. Вы можете использовать несколько стратегий, чтобы исправить это. Вы можете хранить соединения в ThreadLocal или в стеке. Если вы собираетесь хранить соединения в стеке, вам придется открывать и закрывать их при каждом вызове метода. Для достижения некоторой производительности вам нужно будет использовать пул соединений. Пул подключений не повредит в любом случае.

1 голос
/ 09 июля 2011

Нет ничего плохого в том, чтобы делать так для простых приложений. Но если ваше приложение даже умеренно сложное, вы можете захотеть взглянуть на простую инфраструктуру, такую ​​как iBatis.

Пара вещей, которые я определенно сделал бы, хотя. Во-первых, приложение может пропускать соединения, когда генерируется исключение, в зависимости от способа закрытия операторов. Все операторы close должны быть перемещены внутри блоков finally.

Так что вместо:

try {
            Statement stmt = this.connection.createStatement();
            result = stmt.executeUpdate(sql);
            stmt.close();
        } catch (SQLException e) {
            this.handleException(e, sql);
        }

Сделайте это вместо:

Statement stmt = null;
try {
        stmt = this.connection.createStatement();
        result = stmt.executeUpdate(sql);
    } catch (SQLException e) {
        this.handleException(e, sql);
    } finally {
        if (stmt != null) stmt.close();
    }

Другое дело, что я хотел бы убедиться, что вы используете пул соединений с базой данных для своего источника данных. Если вы выполняете это в Tomcat, надеюсь, в установке tomcat определен пул соединений, и ваше приложение использует это.

РЕДАКТИРОВАТЬ: После повторного просмотра кода, я также не вижу, где соединение с базой данных фактически закрывается. Вероятно, поэтому у вас закончились соединения. Вам необходимо добавить метод close в класс Database, который вызывает connection.close (). И убедитесь, что вы звоните, когда закончите с вашими запросами. Снова в блоке try / finally.

Добро пожаловать на сайт PullRequest, где вы можете задавать вопросы и получать ответы от других членов сообщества.
...