Poor code quality in java sdks

Introduction

I’ve noticed a pattern lately in java sdks from large companies that should be hiring experienced developers. I tweeted about this in mid August and was happy to hear back from a Stripe sdk developer (they are working on a fix in their latest release), but I’ve decided its worth a short blog to call out Twilio and shine my tiny flashlight on the problem.

TL;DR

Please don’t use static variables to store credentials in your api client lib if you know there might be use cases when more than one credential might be used concurrently.

The problem

screenshot of tweet

So what am I compaining about exactly…

Global State ☠

When Twilio.init(ACCOUNT_SID, AUTH_TOKEN) or Stripe.apiKey() is called, the jvm is initializing static fields in associated classed. Static fields are class-level variables that are shared among all instances of that class.

Concurrency Risks

Overwriting State: If another thread or process initializes the Twilio class or Stripe class with different credentials, it will overwrite the existing credentials.
Inconsistency: Multiple threads could see inconsistent states if they read the credentials while another thread is modifying it.

Is this really a serious issue for Stripe or Twilio?

Both Twilio and Stripe have immediate reasons to consider multiple credentials in the same runtime (e.g. Twilio has sub accounts and Stripe has Connect accounts), but neither company has taken thread safety seriously in their sdk for java or documentation. It is my view that they are going to be responsible for serious production bugs. In an industry full of novice programmers with on average less than five years of experience I’m confident many will unfortunately not notice the issue and blindly follow the docs.

Broken attempts to consider multiple threads

Picking on Stripe

This snippet of code (see Source)

package com.stripe;
 
import java.net.PasswordAuthentication;
import java.net.Proxy;
import java.util.HashMap;
import java.util.Map;
 
public abstract class Stripe {
  public static final int DEFAULT_CONNECT_TIMEOUT = 30 * 1000;
  public static final int DEFAULT_READ_TIMEOUT = 80 * 1000;
 
  public static final String API_VERSION = ApiVersion.CURRENT;
  public static final String CONNECT_API_BASE = "https://connect.stripe.com";
  public static final String LIVE_API_BASE = "https://api.stripe.com";
  public static final String UPLOAD_API_BASE = "https://files.stripe.com";
  public static final String VERSION = "23.8.0";
 
  public static volatile String apiKey;
  public static volatile String clientId;
  public static volatile boolean enableTelemetry = true;
  public static volatile String partnerId;
  ...

caused me some confusion as it seems like the author is not clear on how the volatile modifier works in the jvm. The volatile keyword ensures that reads and writes to the variable are directly from and to the main memory, bypassing the thread’s local cache. However, this alone does not make the operations on apiKey thread-safe. In fact, I would suggest this makes the problem worse in most cases as it makes it more likely a value written by a thread impacts all other threads (who may have written an alternative value to the same variable).

Picking on Twilio

public class Twilio {
 
    public static final String VERSION = "9.13.1";
    public static final String JAVA_VERSION = System.getProperty("java.version");
    public static final String OS_NAME = System.getProperty("os.name");
    public static final String OS_ARCH = System.getProperty("os.arch");
    private static String username = System.getenv("TWILIO_ACCOUNT_SID");
    private static String password = System.getenv("TWILIO_AUTH_TOKEN");
    private static String accountSid; // username used if this is null
    @Getter
    private static List<String> userAgentExtensions;
    private static String region = System.getenv("TWILIO_REGION");
    private static String edge = System.getenv("TWILIO_EDGE");
    private static volatile TwilioRestClient restClient;
    private static volatile ExecutorService executorService;
 
    private Twilio() {
    }
    ...
    public static synchronized void init(final String username, final String password) {
        Twilio.setUsername(username);
        Twilio.setPassword(password);
        Twilio.setAccountSid(null);
    }
    
    public static synchronized void init(final String username, final String password, final String accountSid) {
        Twilio.setUsername(username);
        Twilio.setPassword(password);
        Twilio.setAccountSid(accountSid);
    }

Using static synchronized in the init method to set username, password, and accountSid in Twilio’s static context does not provide real thread safety. This code demonstrates the lack of deep understanding of the language.

Workarounds

I do want to make a note that there are existing workarounds that allow developers to safely use the java sdks from Twilio and Stripe, but I don’t think this means their example code is any better or more excusable.

Stripe workarounds

To workaround the possibility of concurrency issues with the static apiKey in Stripe there is an optional RequestOptions class that can be passed when making an api call.

RequestOptions options = RequestOptions.builder()
                          .setApiKey(secretKey) // this is where you would pass your thread safe apiKey value
                          .setReadTimeout(READ_TIMEOUT)
                          .setConnectTimeout(CONNECT_TIMEOUT)
                          .build();

Twilio workaround

You can instantiate a new TwilioRestClient object for each request.

Example

  final TwilioRestClient client = new TwilioRestClient.Builder(subaccountSid, twilioSecret).build();
  Page<Message> service = Message.reader().firstPage(client);

Conclusion

While it is undeniably difficult companies of any size to support first class language clients for your APIs, it is my view that it would be better to simply publish your api docs than provide poorly written clients.

A terrible code smell in java sdks