Thread-safe collection to store Member objects [closed]

Question

This is poorly written Java code, intended to implement a thread-safe collection to store Member objects and failing at doing so.

import javax.annotation.concurrent.ThreadSafe;

import java.io.Closeable;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.util.HashSet;

/**
 * A thread-safe container that stores a group ID and members.
 *
 * This class can store Member and/or AdminMember.
 * Also, it can start and stop a background task that writes a member list to specified 2 files.
 *
 * This class is called many times, so it should have a good performance.
 *
 * Example usage:
 * 
 *    MemberGroup group = new MemberGroup("group-001");
 *
 *    group.addMember(new MemberGroup.Member("member-100", 42));
 *    group.addMember(new MemberGroup.AdminMember("admin-999", 30));
 *    group.addMember(new MemberGroup.Member("member-321", 15));
 *
 *    group.startLoggingMemberList10Times("/tmp/output.primary", "/tmp/output.secondary");
 */
@ThreadSafe
public class MemberGroup
    implements Closeable
{
    String groupId;
    HashSet<Member> members;
    boolean isRunning;
    boolean shouldStop;

    class Member
    {
        String memberId;
        int age;

        Member(String memberId, int age)
        {
            this.memberId = memberId;
            this.age = age;
        }

        public String getMemberId()
        {
            return memberId;
        }

        public int getAge()
        {
            return age;
        }

        
        // TODO: check class type too
        public boolean equals(Object o)
        {
            // If `memberId` matches the other's one, they should be treated as the same `Member` objects.
            Member member = (Member) o;
            return this.memberId == member.memberId;
        }
    }

    class AdminMember extends Member
    {
        AdminMember(String memberId, int age)
        {
            super(memberId, age);
        }
    }

    public MemberGroup(String groupId)
    {
        this.groupId = groupId;
        this.members = new HashSet<>();
    }

    public void addMember(Member member)
    {
        members.add(member);
    }

     // TODO: Need to create a Decorator class for print memberId?
    private String getDecoratedMemberId(Member member)
    {
        if (member instanceof Member) {
            return member.getMemberId() + "(normal)";
        }
        else if (member instanceof AdminMember) {
            return member.getMemberId() + "(admin)";
        }
        return null;
    }

    // TODO: Need to create a Decorator class for calculation too?
    private String getMembersAsStringFlooringAge()
    {
        String buf = "";
        for (Member member : members)
        {
            // Floor the age: e.g. 37 -> 30
            Integer flooredAge = (member.getAge() / 10) * 10;
            String decoratedMemberId = getDecoratedMemberId(member);
            buf += String.format("memberId=%s, age=%d¥n", decoratedMemberId, flooredAge);
        }
        return buf;
    }

    @Override
    public void close()
            throws IOException
    {
    }


    /**
     * Run a background task that writes a member list to specified files 10 times in background thread
     * so that it doesn't block the caller's thread.
     *
     * Only one thread is allowed to run at once
     *  - When this method is called and another thread is running, the method call should just return w/o starting any thread
     *  - When this method is called and another thread is already finished, the method call should start a new thread
     */
    /*
    TODO:
        - Should return a CompletableFuture
        - shouldStop should be volatile 
        - Using ExecutorService to support thread pool and to submit multi parallel tasks
        - Create a runnable task name LoggingMembersTask to submit new thread using Threadpool. Each Task writes data to one file and it also check shouldStop to continue or not.
        - Create a Job class as parameter for this startLoggingMemberList10Times method which contains 
                - the list of output file name 
                - the number of times each task should write members to file
        - Each file in this job class is submitted 1 LoggingMembersTask to write output to file 
        - Using CompletableFuture to combine all Future from all submitted task by ExecutorService and return this CompletableFuture.
        - The method should be rename to startLoggingMemberList to make it more generic and configurable.
        - Should throw exception in case if job is summitted and not submit to stop.
     */  
    public void startLoggingMemberList10Times(final String outputFilePrimary, final String outputFileSecondary)
    {
        // Only one thread is allowed to run at once
        if (isRunning) {
            return;
        }
        isRunning = true;

        new Thread(new Runnable() {
            @Override
            public void run()
            {
                int i = 0;
                while (!shouldStop)
                {
                    if (i++ >= 10)
                        break;

                    FileWriter writer0 = null;
                    FileWriter writer1 = null;
                    try {
                        String membersStr = DisappointingGroup.this.getMembersAsStringFlooringAge();

                        writer0 = new FileWriter(new File(outputFilePrimary));
                        writer0.append(membersStr);

                        writer1 = new FileWriter(new File(outputFileSecondary));
                        writer1.append(membersStr);
                    }
                    catch (Exception e) {
                        throw new RuntimeException(
                                "Unexpected error occurred. Please check these file names. outputFilePrimary="
                                        + outputFilePrimary + ", outputFileSecondary=" + outputFileSecondary);
                    }
                    finally {
                        try {
                            if (writer0 != null)
                                writer0.close();

                            if (writer1 != null)
                                writer1.close();
                        }
                        catch (Exception ignored) {
                            // Do nothing since there isn't anything we can do here, right?
                        }
                    }

                    try {
                        Thread.sleep(1000);
                    }
                    catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
            }
        }).start();
    }

    /**
     * Stop the background task started by startLoggingMemberList10Times
     */
    public void stopPrintingMemberList()
    {
        shouldStop = true;
    }
}

I also put some TODO comments which I am going to do to improve this code block. I don't know if I still miss anything else? How to safely stop all threads; is using a boolean flag like this good?

Answer 1

Thread Unsafe

It's not thread-safe. addMember(Member member) directly calls members.add(member); with members being a HashSet. And its documentation says

Note that this implementation is not synchronized. If multiple threads access a hash set concurrently, and at least one of the threads modifies the set, it must be synchronized externally.

So, wrap the HashSet into a Collections.synchronizedSet(...) expression, or use the synchronized keyword on relevant methods, or use other means of making the set access thread-safe.

Making classes thread-safe can be tricky, and testing for thread-safety is close to impossible, so you have to do it right from the start by applying the correct patterns on a theoretical foundation.

Another example of not being thread-safe is your isRunning field. Probably, this is meant to guarantee that there aren't multiple calls of startLoggingMemberList10Times() running in parallel. First of all, you should set it to false somewhere after you finished. But this pattern has so many loopholes where things will go wrong if it's really used in a multi-threading environment that I can't name them all.

I can only recommend to get a good text book on multi-threading and really read through it. You'll learn where things can go wrong and what you can do against it.

Non-OO design

private String getDecoratedMemberId(Member member) makes decisions depending on the class of member, using instanceof. Using instanceof always is an indicator of an OO design flaw. Instead, make that a method of the Member class which can be overridden in AdminMember. Or add a public String getRole() method to the Member class, and use that for the text generation.

Why do you want to have two classes, Member and AdminMember at all? The only difference seems to be in generating some reporting text. This can be done more easily with a new field private String role in the Member class.

If you really want to maintain different classes for the different member roles, then you'd better create an interface Member that defines the methods needed from a Member, and create classes like NormalMember, AdminMember, and maybe GuestMember, all implementing Member.