FtpServer
  1. FtpServer
  2. FTPSERVER-7

Problem when using Database to store/retrieve user info

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Critical Critical
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Environment:
      Installed on Windows 2000 Server.
      Java 1.4.2_04

      Description

      I have been testing the Incubator FTP Server for some time. I have installed it on a Windows 2000 server and configured to store/retrieve user information from an Oracle Database.

      I have observed that sporadically the FTP Server looses connection to the database and that prohibits any authorized user to login to the server. This happens once in a couple of days, and after that if you restat the FTP server, the problem is gone.
      I see the following error message in log files:

      ERROR 2004-10-14 10:31:09.687 [user-man] (): DbUserManager.authenticate()
      java.sql.SQLException: Io exception: Software caused connection abort: socket write error
      at oracle.jdbc.dbaccess.DBError.throwSqlException(DBError.java:114)
      at oracle.jdbc.dbaccess.DBError.throwSqlException(DBError.java:156)
      at oracle.jdbc.dbaccess.DBError.throwSqlException(DBError.java:269)
      at oracle.jdbc.driver.OracleStatement.<init>(OracleStatement.java:292)
      at oracle.jdbc.driver.OracleConnection.createStatement(OracleConnection.java:311)
      at org.apache.ftpserver.usermanager.DbUserManager.authenticate(DbUserManager.java:393)
      at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

      Please let me know if anyone has any thoughts as to why this problem is occurring and how to resolve.

      1. FTPSERVER-7b.txt
        14 kB
        Sergey Vladimirov
      2. FTPSERVER-7b.txt
        14 kB
        Sergey Vladimirov
      3. FTPSERVER-7.txt
        6 kB
        Sergey Vladimirov

        Activity

        Surajit Dash created issue -
        Hide
        Sergey Vladimirov added a comment -

        Be careful: not tested (have no database near me)

        Show
        Sergey Vladimirov added a comment - Be careful: not tested (have no database near me)
        Sergey Vladimirov made changes -
        Field Original Value New Value
        Attachment FTPSERVER-7.txt [ 12320299 ]
        Hide
        Luis Sanabria added a comment -

        This patch will send an additional query to the database server every time a new query is needed. I think it should be better to clean up the connection once the SQL exception is generated. The work flow would be as follows:

        1- A request is processed successfully.
        2- The connection breaks.
        3- Another request is sent to the server
        4- The SQL exception is raised and the connection is clean up.
        5- The user have a rejected log in and retries.

        I think the retry is not a big problem because the invalid connections are uncommon. Additionally, the system can be changed to retry the operation a second time if a SQL exception is generated the first one.

        What do you think?

        Show
        Luis Sanabria added a comment - This patch will send an additional query to the database server every time a new query is needed. I think it should be better to clean up the connection once the SQL exception is generated. The work flow would be as follows: 1- A request is processed successfully. 2- The connection breaks. 3- Another request is sent to the server 4- The SQL exception is raised and the connection is clean up. 5- The user have a rejected log in and retries. I think the retry is not a big problem because the invalid connections are uncommon. Additionally, the system can be changed to retry the operation a second time if a SQL exception is generated the first one. What do you think?
        Hide
        Sergey Vladimirov added a comment -

        I copy my solution from standard pools implementation - they test connection before passing it to user code.

        If user code not completed normally, exception is thrown. Next time before borrowing connection will be checked and closed connection as broken. New connection will be opened and sended to user code.

        But for DbUserManager we not use borrowConnection() template, so

        The best solution for DbUserManager will be to use normal pooling solution, like commons-dbpool, 'cose it thread safe (DbUserManager is not), it provide automatic checking of broken connections (DbUserManager provides only simple - with this patch) and it already builded bycicle

        I'm not agree with checking of every possible sql exception, becose we will need to do it after every connection using, and it will multiply DbUserManager code.

        BTW, depending on database you are using, test sql can be very simple, like "SELECT 1+1" for MySQL or "SELECT 1 FROM DUAL" for Oracle.

        Show
        Sergey Vladimirov added a comment - I copy my solution from standard pools implementation - they test connection before passing it to user code. If user code not completed normally, exception is thrown. Next time before borrowing connection will be checked and closed connection as broken. New connection will be opened and sended to user code. But for DbUserManager we not use borrowConnection() template, so The best solution for DbUserManager will be to use normal pooling solution, like commons-dbpool, 'cose it thread safe (DbUserManager is not), it provide automatic checking of broken connections (DbUserManager provides only simple - with this patch) and it already builded bycicle I'm not agree with checking of every possible sql exception, becose we will need to do it after every connection using, and it will multiply DbUserManager code. BTW, depending on database you are using, test sql can be very simple, like "SELECT 1+1" for MySQL or "SELECT 1 FROM DUAL" for Oracle.
        Hide
        Sergey Vladimirov added a comment -

        Fix - DbUserManager is thread-safe, but it maked thread-safed by adding "synchronized" to every connection-using method.

        Show
        Sergey Vladimirov added a comment - Fix - DbUserManager is thread-safe, but it maked thread-safed by adding "synchronized" to every connection-using method.
        Hide
        Luis Sanabria added a comment -

        The problem is DbUserManager does not use any pool of conections it uses a single connection. When you check for the connection to be valid in a pool implementation you are sending a single transaction for a possible high number of transactions to be sent by the user of the connection. Here the application would be sending a test querry for each query to be executed by the application.

        To check the connection after the exception would be as simple as swiching a flag on the catch clauses and checking it on the prepareConnection method (the problem would be to retry the operation in order to avoid the failed login operation ).

        I am fine with your patch (it solves the issue). I just wanted to post the question. Additionally, I am not sure what is worse, to send the querry or to close and open the connection again on every SQL exception.

        Show
        Luis Sanabria added a comment - The problem is DbUserManager does not use any pool of conections it uses a single connection. When you check for the connection to be valid in a pool implementation you are sending a single transaction for a possible high number of transactions to be sent by the user of the connection. Here the application would be sending a test querry for each query to be executed by the application. To check the connection after the exception would be as simple as swiching a flag on the catch clauses and checking it on the prepareConnection method (the problem would be to retry the operation in order to avoid the failed login operation ). I am fine with your patch (it solves the issue). I just wanted to post the question. Additionally, I am not sure what is worse, to send the querry or to close and open the connection again on every SQL exception.
        Hide
        Sergey Vladimirov added a comment -

        Ok, second variant.

        Store flag "m_connErrorCatched" and sending test query if and only if only when flag==true

        Show
        Sergey Vladimirov added a comment - Ok, second variant. Store flag "m_connErrorCatched" and sending test query if and only if only when flag==true
        Sergey Vladimirov made changes -
        Attachment FTPSERVER-7b.txt [ 12320307 ]
        Hide
        Sergey Vladimirov added a comment -

        Sorry, missed button

        Show
        Sergey Vladimirov added a comment - Sorry, missed button
        Sergey Vladimirov made changes -
        Attachment FTPSERVER-7b.txt [ 12320308 ]
        Hide
        Sergey Vladimirov added a comment -

        Did new patch help you?

        Please, provide your answer about including this patch to SVN for other users.

        Show
        Sergey Vladimirov added a comment - Did new patch help you? Please, provide your answer about including this patch to SVN for other users.
        Hide
        Luis Sanabria added a comment -

        I think it is better to apply the first patch, we can work on it later if performance becomes a problem with performance (I don't think that would be the case). The second one fails the first query after the connection was broken.

        Show
        Luis Sanabria added a comment - I think it is better to apply the first patch, we can work on it later if performance becomes a problem with performance (I don't think that would be the case). The second one fails the first query after the connection was broken.
        Hide
        Rana Bhattacharyya added a comment -

        I love the idea to check the connection using a query. I am wondering what does connection.isClosed() do? Currently we check the connection using isClosed() method.

        Show
        Rana Bhattacharyya added a comment - I love the idea to check the connection using a query. I am wondering what does connection.isClosed() do? Currently we check the connection using isClosed() method.
        Hide
        Sergey Vladimirov added a comment -

        If you like it, please, apply patch and close the most urgent issue

        Show
        Sergey Vladimirov added a comment - If you like it, please, apply patch and close the most urgent issue

          People

          • Assignee:
            Unassigned
            Reporter:
            Surajit Dash
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development