fshutdown / JMeter-WebSocketSampler

JMeter - WebSocket Sampler
Apache License 2.0
309 stars 191 forks source link

Should add synchronization for responeBacklog #43

Open sanjiely opened 8 years ago

sanjiely commented 8 years ago

I got an exception when running performance test, like: null pointer on the line 149 in ServiceSocket.java. The line is: responseMessage += iterator.next();

public String getResponseMessage() {
    String responseMessage = "";

    //Iterate through response messages saved in the responeBacklog cache
    Iterator<String> iterator = responeBacklog.iterator();
    while (iterator.hasNext()) {
        responseMessage += iterator.next();
    }

    return responseMessage;
}

I guess the element had been removed by the poll method in "addResponseMessage" when to fetch it from this iterator. I try to add a "synchronized object" for both this code block and in the method "addResponseMessage", and this issue is resolved. My modifications in "getResponseMessage" :

    synchronized(responeBacklog)
    {
        //Iterate through response messages saved in the responeBacklog cache
        Iterator<String> iterator = responeBacklog.iterator();
        while (iterator.hasNext()) {
            responseMessage += iterator.next();
        }
    }

and in "addResponseMessage":

    synchronized(responeBacklog)
    {
        while (responeBacklog.size() >= messageBacklog) {
            responeBacklog.poll();
        }
        responeBacklog.add(message);
    }

I'm not familiar with Java. Above is just my point. Thanks.

Yong

swordmaster2k commented 8 years ago

We encountered the same issue when testing a webchat application that was exchanging a large amount of messages. The issue was resolved by changing the responseBacklog declaration from a LinkedList which doesn't support concurrent access to a ConcurrentLinkedQueue:

protected Deque<String> responeBacklog = new ConcurrentLinkedQueue<String>();

See https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentLinkedQueue.html

sanjiely commented 8 years ago

Great idea! Let me try. Thanks.

swordmaster2k commented 8 years ago

Quick update that should be a ConcurrentLinkedDequeue:

new ConcurrentLinkedDeque<>();
sanjiely commented 8 years ago

Got it. Thanks.

2015-11-05 19:19 GMT+08:00 Joshua Michael Daly notifications@github.com:

Quick update that should be a ConcurrentLinkedDequeue:

new ConcurrentLinkedDeque<>();

— Reply to this email directly or view it on GitHub https://github.com/maciejzaleski/JMeter-WebSocketSampler/issues/43#issuecomment-154034401 .