Import Dialog hangs when multiple delimiters are selected

[Srikanth Bezawada]

USE CASE: WHAT DO YOU WANT TO DO?

Select multiple delimiters in the data import dialog (Please use File -> import to invoke import dialog)

STEPS TO REPRODUCE AN ISSUE (OR TRIGGER A NEW FEATURE)

1. File -> import -> small_133x133.txt
2. Try to select multiple delimiters

CURRENT BEHAVIOR

Import Dialog becomes non-functional after some seconds

EXPECTED BEHAVIOR

Import Dialog should be functional

DEVELOPERS ONLY SECTION SUGGESTED CHANGE

FILES AFFECTED (where the changes will be implemented) - developers only

DatamportDialog.java, DataImportController.java

LEVEL OF EFFORT - developers only trivial/minor/medium/major/overhaul (choose one)

COMMENTS

1) This might be a temporary minor issue(since we deal with TAB delimited files most of the time) but since the app is getting non-functional, created this issue for reference.

2) Any one else able to reproduce this from latest master? Please find the screenshot attached.

[Christopher Keil]

Confirmed this happening on my machine as well. The issue is an out of memory error. Here is a screenshot of the stacktrace:

Looks almost like an out of control loop or something similar in includeLabelsUpTo(), when multiple delimiters are selected maybe a stop condition breaks.

What is interesting is that the calculations do eventually finish. It is possible that a space delimiter fills the arrays with too much data and a genuine out of memory error happens as opposed to a programmatic problem like a runaway loop. In this case the memory error should be handled with try-catch, error logging, and the new Helper.showError() method.

@abarysh it would probably be good to assign a milestone etc. here so this app-breaking problem can be fixed asap.

[Robert Leach]

I cannot reproduce this on master. I was able to reproduce it on the jar file attached to the PR for issue 444. Has this already been fixed on master or is it OS-specific?

[Robert Leach]

I doubt it's OS-specific now that I think about it - because I can reproduce it given the jar for the PR for issue 444. Is it specific to running from a jar maybe versus running it from the Eclipse run button? Has this issue only ever happened to you guys from the jar built for the PR for issue 444?

[Robert Leach]

Whoops, my bad. I can reproduce this from master. I just hadn't finished my pull. It apparently is from recent changes.

[Christopher Keil]

So what are the changes from master that make this repeoducable? Do you have a terminal log from the output of git pull?

[Robert Leach]

I can't figure out how to see where the pull started. Of course, there had been a window showing all the commits that were pulled, but I had dismissed that. The most I can say is that I didn't pull at all last week, though I'm not sure when the previous time was that I pulled master before then.

[Robert Leach]

If I go back 1 commit, I don't see the bug, but... I'm not sure that the commit history I see in Eclipse is all from the master branch. I could be switching branches for all I know. I suspect I'm switching branches because the last commit I'm skipping back from is the hot fix for gradle which only changed the gradle build script and that doesn't see like it would fix anything about import.

[Robert Leach]

OK, I have isolated it. The bug was introduced to master on commit 09e085f ("Merged in issue-#485 (pull request #121) Changing the delimiter doesn't always trigger auto-detect labels"). Every merge to master there and there-after has the bug and every commit to master before it does not. I.e. 6cfda85 - ("Merged in issue-#398 (pull request #117) "Autodetect labels" button misidentifies label columns when first data cell empty") does not have the bug. I suspect an endless loop? Because all that was done was adding calls to detectDataBoundaries(null).

[Srikanth Bezawada]

Chris, Have you tried inspecting merged PR's and the jars linked with each of them ?

I couldn't reproduce until PR#121, For PR-119 jar was unavailable and from PR-124 onwards it is reproducible.

[Robert Leach]

I just went dow the commits merged to master on the commits page and kept checking them out and testing until the bug was gone. I agree, it's in PR 121, the merging of the branch for issue 485.

[Robert Leach]

I would put my money on this scenario:

Clicking a delimiter checkbox calls detectDataBoundaries(null) from inside actionPerformed. detectDataBoundaries calls setSpinnerValues when the PreviewDialog is not null, which in turn probably initiates a call to actionPerformed? I.e. an endless loop?

[Robert Leach]

Hmmm... it only happens with the spaces checkbox, so it's not as simple as I initially thought. Also - if you deselect every checkbox and then check the spaces checkbox, you still encounter the bug, so it doesn't have to do with "when multiple delimiters are selected" - only spaces.

[Robert Leach]

This line in DataImportDialog is where the hang occurs:

columnDataStart.setValue(Integer.valueOf(columnCount) + 1);

The value of columnCount (after unchecking tab and checking spaces) is 2147483647 according to my debug prints.

[Robert Leach]

Alright, this is a combination of a couple changes. The bug may have been introduced into master with PR 121,but it looks to me like the recent change inside findDataStartCoords where the starting data column is initially set to Integer.MAX_VALUE is never checked to ensure that an actual good value was found. I believe that was probably part of the same PR. You'd have to march back through the commits on that branch to catch the change that did it, but I think this is the cause. I think that if a starting data column cannot be found given the selected delimiters, the default starting data column should be set to column 0 (or from the user's perspective: column 1). They will hit an error when they try to continue. Alternatively, we could detect this situation and disable to continue button until the user manually changes the data start? I'm not certain what the best solution is. I don't think we should pop up an error in the middle of the user interacting with the interface.

[Christopher Keil]

I think we should correct the start coordinates for data to (0,0) if no data is found. The opposite as it happens now really does make no sense.

As for the JSpinners the code for updating them should be removed from detectDataBoundaries and only be used when the labels are auto-detected on first load and when pressing the button. This should break the infinite loop. The method returns the needed values for the spinners anyways.

[Robert Leach]

Agreed

[Robert Leach]

Starting work.

[Robert Leach]

Looks like this is already fixed in master. Yay!

[Robert Leach]

This was already fixed in master.