Java Servlet Method Parameters, response, and thread safety

Question

I have written some REST APIs using Java Servlets on Tomcat. These are my first experiences with Java and APIs and Tomcat. As I research and read about servlets, methods and parameter passing, and more recently thread safety, I realize I need some review, suggestions, and tutorial guidance from those of you who I see are far more experienced. I have found many questions / answers that seem to address pieces but my lack of experience clouds the clarity I desire.

The code below shows the top portion of one servlet example along with an example private method. I have "global" variables defined at the class level so that I may track the success of a method and determine if I need to send an error response. I do this because the method(s) already return a value.

1. Are those global variables creating an unsafe thread environment
2. Since the response is not visible in the private methods, how else might I determine the need to stop the process and send an error response if those global variables are unsafe
3. Though clipped for space, should I be doing all of the XML handling in the doGet method
4. Should I be calling all of the different private methods for the various data retrieval tasks and data handling
5. Should each method that accesses the same database open a Connection or should the doGet method create a Connection and pass it to each method

Assist, suggest, teach, guide to whatever you feel appropriate, or point me to the right learning resources so I may learn how to do better. Direct and constructive criticism welcome -- bashing and derogatory statements not preferred.

@WebServlet(name = "SubPlans", urlPatterns = {"*omitted*"})
public class SubPlans extends HttpServlet {

  private transient ServletConfig servletConfig;
  private String planSpecialNotes,
      planAddlReqLinks,
      legalTermsHeader,
      legalTermsMemo,
      httpReturnMsg;
  private String[] subPlanInd = new String[4];
  private boolean sc200;
  private int httpReturnStatus;

  private static final long serialVersionUID = 1L;

  {
    httpReturnStatus = 0;
    httpReturnMsg = "";
    sc200 = true;
    planAddlReqLinks = null;
    planSpecialNotes = null;
    legalTermsHeader = "";
    legalTermsMemo = null;
  }

  @Override
  public void init(ServletConfig servletConfig)
      throws ServletException {
    this.servletConfig = servletConfig;
  }

  @Override
  public ServletConfig getServletConfig() {
    return servletConfig;
  }

  @Override
  public String getServletInfo() {
    return "SubPlans";
  }

  @Override
  public void doGet(HttpServletRequest request, HttpServletResponse   response)
      throws ServletException, IOException {
    List<HashMap<String, Object>> alSubDeps = new ArrayList<HashMap<String, Object>>();
    String[] coverageDates = new String[6],
        depDates = new String[8];
    String eeAltId = null,
        eeSSN = null,
        carrier = null,
        logosite = null,
        fmtSSN = "X",
        subSQL = null,
        healthPlan = null,
        dentalPlan = null,
        visionPlan = null,
        lifePlan = null,
        tier = null,
        healthGroupNum = null,
        effdate = null,
        holdEffDate = null,
        planDesc = "",
        planYear = "",
        summaryBenefitsLink = null;
    int[][] effdates = new int[6][4];
    int holdDistrictNumber = 0,
        districtNumber = 0,
        holdUnit = 0,
        unit = 0;
    boolean districtHasHSA = false;
    XMLOutputFactory outputFactory = XMLOutputFactory.newInstance();
    try {
      eeAltId = request.getParameter("*omitted*");
      if ( eeAltId != null ) {
        Pattern p = Pattern.compile(*omitted*);
        Matcher m = p.matcher(eeAltId);
        if ( m.find(0) ) {
          eeSSN = getSSN(eeAltId);
        } else {
          httpReturnStatus = 412;
          httpReturnMsg = "Alternate ID format incorrect.";
          System.err.println("Bad alternate id format " + eeAltId);
          sc200 = false;
        }
      } else {
        httpReturnStatus = 412;
        httpReturnMsg = "Alternate ID missing.";
        System.err.println("alternate id not provided.");
        sc200 = false;
      }
      if ( sc200 ) {
        coverageDates = determineDates();
        subSQL = buildSubSQLStatement(eeSSN, coverageDates);
        alSubDeps = getSubDeps(subSQL);
        if ( sc200 ) {
          XMLStreamWriter writer = outputFactory.createXMLStreamWriter(response.getOutputStream());
          writer.writeStartDocument("1.0");
          writer.writeStartElement("subscriber");
     //  CLIPPED //
          writer.writeEndElement();   // subscriber
          writer.writeEndDocument();
          if ( sc200 ) {
            response.setStatus(HttpServletResponse.SC_OK);
            writer.flush();
          } else {
            response.sendError(httpReturnStatus, httpReturnMsg);
          }
        }
      }
    } catch (Exception e) {
      e.printStackTrace();
      System.err.println("Error writing XML");
      System.err.println(e);
    }
  }
  @Override
  public void destroy() {

  }

  private String getPlanDescription(String planID) {
    String planDesc = null;
    String sqlEE = "SELECT ...";
    Connection connGPD = null;
    Statement stGPD = null;
    ResultSet rsGPD = null;
    try {
      connGPD = getDbConnectionEE();
      try {
        stGPD = connGPD.createStatement();
        planDesc = "Statement error";
        try {
          rsGPD = stGPD.executeQuery(sqlEE);
          if ( !rsGPD.isBeforeFirst() )
            planDesc = "No data";
          else {
            rsGPD.next();
            planDesc = rsGPD.getString("Plan_Description");
          }
        } catch (Exception rsErr) {
          httpReturnStatus = 500;
          httpReturnMsg = "Error retrieving plan description.";
          System.err.println("getPlanDescription: " + httpReturnMsg + " " + httpReturnStatus);
          System.err.println(rsErr);
          sc200 = false;
        } finally {
          if ( rsGPD != null ) {
            try {
              rsGPD.close();
            } catch (Exception rsErr) {
              System.err.println("getPlanDescription: Error closing result set.");
              System.err.println(rsErr);
            }
          }
        }
      } catch (Exception stErr) {
        httpReturnStatus = 500;
        httpReturnMsg = "Error creating plan description statement.";
        System.err.println("getPlanDescription: " + httpReturnMsg + " " + httpReturnStatus);
        System.err.println(stErr);
        sc200 = false;
      } finally {
        if ( stGPD != null ) {
          try {
            stGPD.close();
          } catch (Exception stErr) {
            System.err.println("getPlanDescription: Error closing query statement.");
            System.err.println(stErr);
          }
        }
      }
    } catch (Exception connErr) {
      httpReturnStatus = 500;
      httpReturnMsg = "Error closing database.";
      System.err.println("getPlanDescription: " + httpReturnMsg + " " + httpReturnStatus);
      System.err.println(connErr);
      sc200 = false;
    } finally {
      if ( connGPD != null ) {
        try {
          connGPD.close();
        } catch (Exception connErr) {
          System.err.println("getPlanDescription: Error closing connection.");
          System.err.println(connErr);
        }
      }
    }
    return planDesc.trim();
  }

Answer 1

The variables you have defined are instance members. They are not global and are not class-level. They are variables scoped to one instance of your servlet class.

The servlet container typically creates one instance of your servlet and sends all requests to that one instance. So you will have concurrent requests overwriting these variables’ contents unpredictably.

It can be ok for a servlet to have static variables or instance member variables, but only if their contents are thread safe and they contain no state specific to a request. For instance it would be normal to have a (log4j or java.util.logging) Logger object as a static member, where the logger is specifically designed to be called concurrently without the threads interfering with each other.

For error handling use exceptions to fail fast once something goes wrong.

Servlets are painful to write and hard to test. Consider using a MVC web framework instead. Frameworks like spring or dropwizard provide built-in capabilities that make things like data access and error handling easier, but most importantly they encourage patterns where you write separate well-focused classes that each do one thing well (and can be reasoned about and tested independently). The servlet approach tends to lead people to cram disparate functions into one increasingly-unmanageable class file, which seems to be the road you’re headed down.

Answer 2

I have "global" variables defined at the class level

You have instance variables declared at the class level. There are no globals in Java.

so that I may track the success of a method and determine if I need to send an error response.

Poor technique.

I do this because the method(s) already return a value.

You should use exceptions for this if the return values are already taken.

Are those global variables creating an unsafe thread environment

Those instance variables are creating an unsafe thread environment.

Since the response is not visible in the private methods, how else might I determine the need to stop the process and send an error response if those global variables are unsafe?

Via exceptions thrown by the methods, see above. If there is no exception, send an OK response, whatever form that takes, otherwise whatever error response is appropriate to the exception.

Though clipped for space, should I be doing all of the XML handling in the doGet method

Not if it's long or repetitive (used in other places too).

Should I be calling all of the different private methods for the various data retrieval tasks and data handling

Sure, why not?

Should each method that accesses the same database open a Connection or should the doGet() method create a Connection and pass it to each method

doGet() should open the connection, pass it to each method, and infallibly close it.

NB You don't need the ServletConfig variable, or the init() or getServletConfig() methods. If you remove all this you can get it from the base class any time you need it via the getServletConfig() method you have pointlessly overridden.