Skip to content

Fix: Make concurrent CSV Read function really thread safe #6694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
FSchumacher wants to merge 1 commit into
base: master
Choose a base branch
from
+15 −9
Open

Fix: Make concurrent CSV Read function really thread safe #6694

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions src/functions/src/main/java/org/apache/jmeter/functions/FileRowColContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.ArrayList;
import java.util.List;
import java.util.StringTokenizer;
import java.util.concurrent.atomic.AtomicInteger;

import org.apache.jmeter.services.FileServer;
import org.apache.jmeter.util.JMeterUtils;
Expand All @@ -50,7 +51,7 @@ public class FileRowColContainer {
","); // $NON-NLS-1$

/** Keeping track of which row is next to be read. */
private int nextRow;
private final AtomicInteger nextRow;

/** Delimiter for this file */
private final String delimiter;
Expand All @@ -59,7 +60,7 @@ public FileRowColContainer(String file, String delim) throws IOException, FileNo
log.debug("FRCC({},{})", file, delim);
fileName = file;
delimiter = delim;
nextRow = 0;
nextRow = new AtomicInteger(0);
fileData = new ArrayList<>();
load();
}
Expand All @@ -68,7 +69,7 @@ public FileRowColContainer(String file) throws IOException, FileNotFoundExceptio
log.debug("FRCC({})[{}]", file, DELIMITER);
fileName = file;
delimiter = DELIMITER;
nextRow = 0;
nextRow = new AtomicInteger(0);
fileData = new ArrayList<>();
load();
}
Expand Down Expand Up @@ -119,13 +120,18 @@ public String getColumn(int row, int col) throws IndexOutOfBoundsException {
*
*/
public int nextRow() {
int row = nextRow;
nextRow++;
if (nextRow >= fileData.size()) {// 0-based
nextRow = 0;
int size = fileData.size();
if (size == 0) {
throw new IllegalStateException("CSV file " + fileName + " has no data rows");
}
while (true) {
int current = nextRow.get();
int next = (current + 1) % size;
Comment on lines +127 to +129
Copy link
Copy Markdown
Collaborator

@vlsi vlsi Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT of going with AtomicLong.getAndIncrement and java.lang.Math#floorMod(long, int)?
Then it could work without a loop.

E.g.

private final AtomicLong nextRow = new AtomicLong();

      long row = nextRow.getAndIncrement();
      int result = (int) Math.floorMod(row, size);

Copy link
Copy Markdown
Contributor Author

@FSchumacher FSchumacher Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretical we could still run into an overflow and in that case, we would get wrong values ;)

And probably more to the point: the name nextRow is misleading. It should probably be nextRowToReturn or such, as it contains the number, the function should return and the function sets it to the value the next user should read.

Therefore I think the loop is not that badly written.

If we want to take the AtomicLong-road, we should probably start with a value of -1 instead of 0, so that the first row is still read. But in that case we would have to check all uses of the value of nextRow outside of the changed function.

Copy link
Copy Markdown
Collaborator

@vlsi vlsi Apr 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Math.floorMod resolves the overflow, so I don't understand what you mean by "wrong values"

Copy link
Copy Markdown
Contributor Author

@FSchumacher FSchumacher Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

floorMod should work well within the range of long. But when our long wraps around, the calculation will be wrong.

It might be clearer, when we use a short instead of a long.
There we have (x % 255) % FILE_SIZE as our current value. Further assume that FILE_SIZE is 200.
Then for x == 254 we get 54 and for x == 255 we get 0 (wrap around of short).

The same will happen (really unlikely, as a long can get quite big) with an int or a long. (That might will happen with a long after 286.967.718 years, when we read about 1000 rows a second (for that complete period) :) ) (and that will be one wrong range, that we skip, in the above short example 55..255)

Copy link
Copy Markdown
Collaborator

@vlsi vlsi Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clarify what exactly would go wrong.
0 is a valid value, isn't it?

Copy link
Copy Markdown
Contributor Author

@FSchumacher FSchumacher Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing wrong with the zero per se, but it should have been the 55 in the example I gave.

And we still would have the other problem, that we would have to either decrement the value before returning or start with -1 and look at other uses of nextRow, which doesn't feel right to me, either.

if (nextRow.compareAndSet(current, next)) {
log.debug("Row: {}", current);
return current;
}
}
log.debug("Row: {}", row);
return row;
}

/**
Expand Down
Loading