Read dataset files, should we handle all possible exceptions?

[Sandra_Rossi]

Hi, probably a dumb question, but I'm just wondering why we, ABAP developers, usually never handle the documented catchable exceptions while doing OPEN DATASET, READ DATASET, CLOSE DATASET.

Differently said, could you tell me what you think is not well coded in the below code snippet (about the handling of exceptions), and why:

CLASS lcl_app DEFINITION.
  PUBLIC SECTION.
    CLASS-METHODS read_text_file
      IMPORTING
        dataset      TYPE csequence
      RETURNING
        VALUE(lines) TYPE string_table
      RAISING
        cx_sy_file_open
        cx_sy_codepage_converter_init
        cx_sy_conversion_codepage
        cx_sy_file_authority
        cx_sy_file_io
        cx_sy_file_close.
ENDCLASS.


CLASS lcl_app IMPLEMENTATION.
  METHOD read_text_file.
    DATA: line TYPE string.

    " open the UTF-8 text file for reading it
    OPEN DATASET dataset FOR INPUT IN TEXT MODE ENCODING DEFAULT.
    IF sy-subrc <> 0.
      RAISE EXCEPTION TYPE cx_sy_file_open EXPORTING filename = dataset
                            errortext = 'File not found' ##NO_TEXT.
    ENDIF.

    DO.
      " read the next text line
      READ DATASET dataset INTO line.
      IF sy-subrc <> 0.
        EXIT. " end of file -> leave the loop
      ENDIF.
      APPEND line TO lines.
    ENDDO.

    CLOSE DATASET dataset.

  ENDMETHOD.
ENDCLASS.

START-OF-SELECTION.
  TRY.
      DATA(lines) = lcl_app=>read_text_file( '/tmp/myfile' ).
    CATCH cx_root INTO DATA(lx_root).
  ENDTRY.

Thanks a lot 🙂

[fabianlupa]

Catching CX_ROOT is in almost all cases a very bad idea. I personally had one case where I searched for days for the origin of a problem and it turned out that a CATCH CX_ROOT clause swallowed a database deadlock error, just because the developer was too lazy to search for the exception he actually wanted to catch (/endrant).

Otherwise I agree, the exceptions around the file system interface should be handled in some way or another. At work we have a (mockable) wrapper library around the file system statements that reraise all the exceptions you listed in a /.../CX_BC_IO_ERROR which is inherited from CX_STATIC_CHECK, so that there is a syntax warning if you do not handle them. This also helps because you don't have to write down all the exceptions that are possible, as there is no common super class. Also, like you noted, you can only find them in the documentation, there is no autocomplete or anything like that.

[matt]

I've know worse.

TRY.
  ... something....
  CATCH cx_root.
ENDTRY.

and

IF sy-subrc IS NOT INITIAL. ENDIF.

[chaouki_akir]

Sorry but i don't see the link between "CATCH cx_root" and between a DB dead lock.

"CATCH cx_root" is not enough solely to provoke a DB dead lock. An other error may have been done, no ? Do you remember ?

[matt]

You misunderstand. The CATCH cx_root didn't cause the DB lock - it caught it, which made problem diagnosis difficult. The programmer should have caught and handled a more specific exception.

[fabianlupa]

I see your nightmare and raise you another one:

METHOD at_least_it_is_a_method.
  ...
  TRY.
      CALL FUNCTION 'SOMETHING'
        EXCEPTIONS
          OTHERS = 1.
    CATCH cx_root ##NO_HANDLER ##CATCH_ALL.
  ENDTRY.

  CHECK sy-subrc = 0.
ENDMETHOD.

Though your multiline subrc handling (or lack thereof) is terrifying as well

[Sandra_Rossi]

Thank you Fabian. In my code, I forgot to define a logic to handle the exception. Any exception should stop the program and warn the user (or be logged or whatever). The best I could do is to catch CX_DYNAMIC_CHECK (the top-most exception class). Do you think it's really an error to catch ANY error (CX_ROOT) in my code? For instance imagine there could be some CX_NO_CHECK errors (difficult to be sure there can be these ones or not), I'd like to catch them too, because I want to warn the user for any error, rather than generating a runtime error.

[fabianlupa]

I always look at it like this: What can you expect the caller of your method to handle in terms of exceptions? Regarding the file system statements in some cases the caller of a method might not even know the file system is used because it is an implementation detail (debug trace file for example) and not part of the "business logic" the method provides. So you might not want to expose the exceptions directly but "change the perspective" by wrapping them in another exception. Sometimes you also have no choice because you implement an interface where you cannot just add another exception to its methods declarations.

Regarding CX_NO_CHECK: I only use these for internal errors that I don't want anyone to catch. In your example: Do you really want your user to get a message at the bottom of SAP GUI saying "Fatal internal error, data loss imminent." and not tell anyone about it or would you prefer the program created a short dump because the exception was not caught and the basis team was informed automatically via an alert and you can analyze the error because there is a ST22 entry?

[Sandra_Rossi]

That makes sense. In fact, there are two schools because the short dump shown to the user is really ugly, that would be nice to be able to customize the display. It's why people often tells us to catch all possible exceptions, to avoid that screen, and my question was about to go in that direction.

[NTeunckens]

Read up on the Documentation of the OPEN / READ / CLOSE DATASET (F1) and see what Exceptions are mentioned ...

Incorporate those in your Error / Exception Handling.

[Sandra_Rossi]

Thank you. All the exceptions I have mentioned are those from the documentation. My point is more about, well any of those errors could happen (code page issue for instance), I don't want to make the program fail, just warn the user.

[matt]

I always write my code with the intentions of handling all exceptions. Not handling exceptions should be a criminal offence.

I follow Jacques approach below -> but may catch a few specific exceptions, if they require slightly different handling.

[nomssi]

No.

The advantage of exceptions is to separate "normal" code from "error handling". You avoid the problem of return codes that must be checked everywhere in your logic.

An object only handle an exception if it feels responsible for the error (e.g., it has enough context information to decide how to proceed). If not, it should pass the exception to the next handler (propagate the exception). To simplify this, only declare the more abstract exception type in the RAISING clause and use the concrete exception type in the CATCH clause.

I would

a) check the exception hierarchy and only declare the highest level exception CX_DYNAMIC_CHECK, maybe also

CX_SY_FILE_ACCESS_ERROR
b) add exception handling at the highest level, e.g. print an error message with MESSAGE lx_root TYPE 'I'.

[retired_member]

Bonne réponse ...

The exception hierarchy is here:

https://help.sap.com/doc/abapdocu_752_index_htm/7.52/en-US/index.htm?file=abenabap_exception_classes...

Catch specifically, but only as specific as you really want to handle.

https://help.sap.com/doc/abapdocu_752_index_htm/7.52/en-US/index.htm?file=abenhandl_prop_except_guid...

[Sandra_Rossi]

Thank you Jacques (sorry you're right, I forgot MESSAGE lx_root TYPE 'I')

Imagine I define this method in a global class instead, so that any developer can use it. I find it a little bit unclear to only enter the exception CX_DYNAMIC_CHECK (maybe it's the reason why you prefer to add CX_SY_FILE_ACCESS_ERROR?)

Do you think I should just add an ABAP doc comment for that very abstract exception class in the signature of the method? (I guess it doesn't make sense to create a global exception class ZCX_FILE_ACCESS_ERROR just for this one case)

(addendum: I have just seen the answer by Fabian, his solution is to create a global exception class, but it's probably because he has a library with probably many methods)

[nomssi]

Hello Sandra,

I would only use CX_DYNAMIC_CHECK and document it.

But I see a OO design issue. You want to publish a READ_TEXT_FILE as a function module, then you can signal errors with return codes. The alternative could be to publish a FILE class with

• a private filename attribute filled by an constructor importing a FileName parameter
• an instance method READ_TEXT( ) method that can raise an exception of type CX_DYNAMIC_CHECK

The client first creates an object of the type FILE. If successfull, it calls READ_TEXT( ) or other methods. To avoid exceptions, you could use a factory returning either a valid FILE object or a special NULL object (on errors) that has acceptable behavior (e.g. returns empty data) if needed in your scenario.

JNN

[Sandra_Rossi]

It's nice to see the slightly-varying answers among participants. All make sense to me.

You're right, a FILE class would be better. It was more an example here, to discuss about which exceptions should be handled and where, and the case about how to handle ABAP exceptions was interesting too. I never thought about those dummy-exception-tolerant objects, it seems weird to me now, I keep them in mind, maybe one day. Until now I could always deal with exceptions.

[pokrakam]

The obvious fail is the fact that exceptions are being explicitly suppressed. If you use ATC / SLIN and include check 'Problematic statements', it should flag that one up. Preferably priority one and block the transport 🙂

A lesser offence is catching INTO DATA(something) without further use of the object - what's the point?

And there's not much benefit in declaring dynamic exceptions. If it's a known risky operation and you want to enforce error handling, then rather convert them into static exceptions:

CLASS lcx_file_error DEFINITION
                     INHERITING FROM cx_static_check.
ENDCLASS.

CLASS lcl_app DEFINITION.
  PUBLIC SECTION.
    CLASS-METHODS read_text_file IMPORTING dataset      TYPE csequence
                                 RETURNING VALUE(lines) TYPE string_table
                                 RAISING   lcx_file_error.
ENDCLASS.

CLASS lcl_app IMPLEMENTATION.
  METHOD read_text_file.
    DATA: line TYPE string.

    TRY.
        " open the UTF-8 text file for reading it
        OPEN DATASET dataset FOR INPUT IN TEXT MODE ENCODING DEFAULT.
        IF sy-subrc <> 0.
          RAISE EXCEPTION TYPE cx_sy_file_open
            EXPORTING
              filename  = dataset
              errortext = 'File not found' ##NO_TEXT.
        ENDIF.

        DO.
          " read the next text line
          READ DATASET dataset INTO line.
          IF sy-subrc <> 0.
            EXIT. " end of file -> leave the loop
          ENDIF.
          APPEND line TO lines.
        ENDDO.

        CLOSE DATASET dataset.

      CATCH cx_sy_file_open
            cx_sy_codepage_converter_init
            cx_sy_conversion_codepage
            cx_sy_file_authority
            cx_sy_file_io
            cx_sy_file_close.
        RAISE EXCEPTION TYPE lcx_file_error.
    ENDTRY.

  ENDMETHOD.

This is just a quick idea, in real life I'd enhance lcx_file_error to pass on more info.

[Sandra_Rossi]

Thank you Mike. Sorry, I forgot the one line MESSAGE lx_root TYPE 'I' in my code.

You're right, a dedicated exception class is better, rather than using only CX_DYNAMIC_CHECK. If I had my method in a global class instead, then maybe CX_DYNAMIC_CHECK is better than creating a dedicated global exception class, if it's used only by one method, and I would then use ABAP Doc to document what CX_DYNAMIC_CHECK is for (see my comment on Jacques answer).

[pokrakam]

Why not? I'm quite happy to create global exception classes for a single specific use. It makes it clear, you get the benefits of a static check, a where-used leads to exactly the spot where it failed, and you can add all sorts of data. You can organize exception classes in a hierarchy so clutter is not a big issue either.

It may not apply here, but I also tend to be selective in catching exceptions. Some things should really not happen and I'm quite happy to let them dump. It's effort vs probability vs impact. Usually I'll add a comment along the lines of "Serious failure, problem with xyz or a more generic "If this fails then we already have bigger problems

Edit: One more option is the newish MESSAGE parameter. If you really don't like the idea of a global exception class, use a more generic one and add a message:

RAISE EXCEPTION TYPE zcx_general_failure
    MESSAGE ID 'ZFILE' NUMBER '014' 
      WITH filename position.

But personally I still prefer a specific class.

[Sandra_Rossi]

It doesn't cost a lot, so why not 🙂 I prefer having a dedicated exception class too.

[Sandra_Rossi]

Addendum to my post, just to say that I caught CX_ROOT to display the message to the user: CATCH cx_root INTO DATA(lx_root) + MESSAGE lx_root TYPE 'I' + STOP/RETURN.