1

What am I trying to do?

When 2+ front-ends requests something via socket emit, I only want the back-end to process 1 of those requests and reject the other requests.

What is the code that currently tries to do that?

Backend code.

// Original code.

public IoMessage create(IoMessage request) {
  Adventure[] adventures = null;
  AdventureIoMessage createRequest = null;
  StringBuilder message = new StringBuilder();

  // Thinking of putting some "locking" code around this if statement?
  // Something like "If this function is already in process, deny other requests and remove them"?
  if (request instanceof AdventureIoMessage) {
    createRequest = (AdventureIoMessage) request;
    DatabaseManager databaseManager = Application.getDatabaseManager();
    Adventure adventure = Adventure.read(databaseManager);
    if (adventure == null || adventure.getStartTime() == 0 || !adventure.isActive()) {
      createAdventure(adventures, createRequest, message);
    } else {
      message.append("New adventure already exists and hasn't ended yet!");
    }
  } else {
    message.append("Request formatting is invalid!");
  }

  return new AdventureIoMessage(adventures, createRequest, message); 
}
// My suggested code 1 using a Boolean.

public IoMessage create(IoMessage request) {
  Adventure[] adventures = null;
  AdventureIoMessage createRequest = null;
  StringBuilder message = new StringBuilder();
  Boolean isRequestAlreadyRunning = false;
  
  if (!isRequestAlreadyRunning) {
    isRequestAlreadyRunning = true;
    if (request instanceof AdventureIoMessage) {
      createRequest = (AdventureIoMessage) request;
      DatabaseManager databaseManager = Application.getDatabaseManager();
      Adventure adventure = Adventure.read(databaseManager);
      if (adventure == null || !adventure.isActive()) {
        createAdventure(adventures, createRequest, message);
      } else {
        message.append("New adventure already exists and hasn't ended yet!");
      }
    } else {
      message.append("Request formatting is invalid!");
    }
    isRequestAlreadyRunning = false; // Done processing.
  }

  return new AdventureIoMessage(adventures, createRequest, message); 
}
// My suggested code 2 using AtomicBoolean and CountDownLatch.

public IoMessage create(IoMessage request) {
  Adventure[] adventures = null;
  AdventureIoMessage createRequest = null;
  StringBuilder message = new StringBuilder();
  AtomicBoolean updateStarted = new AtomicBoolean();
  CountDownLatch updateFinished = new CountDownLatch(1);
  
  if (updateStarted.compareAndSet(false, true) {
    if (request instanceof AdventureIoMessage) {
      createRequest = (AdventureIoMessage) request;
      DatabaseManager databaseManager = Application.getDatabaseManager();
      Adventure adventure = Adventure.read(databaseManager);
      if (adventure == null || !adventure.isActive()) {
        createAdventure(adventures, createRequest, message);
      } else {
        message.append("New adventure already exists and hasn't ended yet!");
      }
    } else {
      message.append("Request formatting is invalid!");
    }
    updateFinished.countDown();
  } else {
    updateFinished.await();
  }

  return new AdventureIoMessage(adventures, createRequest, message); 
}

For suggested code 2, also tried just the AtomicBoolean only.

What do I expect the result to be?

I thought about keeping some sort of variable to check if the create() should still happen or not, expecting only 1 request to go through.

What is the actual result?

When 2+ front-ends requests something via socket emit, 2 of the create() happens despite the check logic within.

What I think the problem could be?

A timing issue because 2+ front-ends can access this create() method at the same time without a proper check. The original logic of the code works in the process that they don't fire at the same time. I'm hoping this makes sense because it was difficult to word this and new to Java.

EDIT:

My working solution I finalized with:

// Within a class...

AtomicBoolean isRequestAlreadyRunning = new AtomicBoolean();

public IoMessage create(IoMessage request) {
  Adventure[] adventures = null;
  AdventureIoMessage createRequest = null;
  StringBuilder message = new StringBuilder();
  
  if (updateStarted.compareAndSet(false, true) {
    try {
      if (request instanceof AdventureIoMessage) {
        createRequest = (AdventureIoMessage) request;
        DatabaseManager databaseManager = Application.getDatabaseManager();
        Adventure adventure = Adventure.read(databaseManager);
        if (adventure == null || !adventure.isActive()) {
          createAdventure(adventures, createRequest, message);
        } else {
          message.append("New adventure already exists and hasn't ended yet!");
        }
      } else {
        message.append("Request formatting is invalid!");
      }
    } finally {
      isRequestAlreadyRunning.set(false);
    }
  }

  return new AdventureIoMessage(adventures, createRequest, message); 
}
ctrlaltdeleon
  • 356
  • 3
  • 14

1 Answers1

1

Problem: In Java, each method has its own stack, so all the Variables allocated INSIDE the method are unique.

Solution:

  • put your isRequestAlreadyRunning as a member variable, not a method variable
  • make it boolean isRequestAlreadyRunning, NOT Boolean isRequestAlreadyRunning
  • make it volatile, as in private volatile boolean isRequestAlreadyRunning

Then your first try should work.

Your second try will not work for the same reasons.

Alternatively, search for a Java mutex, as in "mutually exclusive", and you'll find many classes and techniques specialized on that, besides the sychronized(myLockObj) statement.

JayC667
  • 2,418
  • 2
  • 17
  • 31
  • 2
    Unfortunately, a `volatile boolean` is not enough to make this work reliably. It's still possible for two threads to read the value as `false` before one updates it to `true`. An `AtomicBoolean` or a mutex such as `java.util.concurrent.locks.ReentrantLock` is the way to go. – Tim Moore Sep 02 '21 at 04:07
  • 1
    @TimMoore true. – JayC667 Sep 02 '21 at 11:14
  • I tried both `AtomicBoolean` and the `ReentrantLock` variable types and for my needs I went with the `AtomicBoolean` as I still want concurrent updates occurring for other methods while `ReentrantLock` will not allow those concurrent updates while in "locked" mode. Hoping I'm grasping the concepts correctly. – ctrlaltdeleon Sep 02 '21 at 16:45
  • If you use `AtomicBoolean` or `ReentrantLock ` like indicated in your code above, you would block ANY parallel creation of adventures. – JayC667 Sep 02 '21 at 17:37
  • If you want to enable partial parallel creation and have a unique identifier (name or hash), you could also use any `ConcurrentMap` (like `ConcurrentNavigableMap` or `ConcurrentHashMap` or `ConcurrentSkipListMap`) to map your identifier, so that any second parallel access to the **same** adventure is controlled. Use methods like `getOrDefault` or `computeIfAbsent` or `computeIfPresent` or `putIfAbsent` to achieve that. If need be, you could even combine that with `AtomicBoolean` by mapping your identifier to the AtomicBoolean like so: `ConcurrentHashMap`. – JayC667 Sep 02 '21 at 17:47