Updated Visitor function to avoid modifying config as it needs to remain

[ivakegg]

thread safe

[FineAndDandy]

can you please add a unit test that covers this case?

[ivakegg]

can you please add a unit test that covers this case?

Since this is fixing a thread safety issue, I am not sure how to actually do that. There are already tests for the large list visitor and the visitor function. What exactly are you thinking here?

[FineAndDandy]

Maybe I should restate that. We should either add some tests that prove the code is thread safe, or update the code to make it more difficult to do thread unsafe activities like modifying the config inside the VisitorFunction. We could prevent changes to the config via an immutable copy being made inside the VisitorFunction, or alternatively, creating a new interface on the config which is read only and then using that inside the VisitorFunction? Either way we create a programmatic barrier to a regression if we aren't going to create tests that validate the bug/assumption?

[ivakegg]

Maybe I should restate that. We should either add some tests that prove the code is thread safe, or update the code to make it more difficult to do thread unsafe activities like modifying the config inside the VisitorFunction. We could prevent changes to the config via an immutable copy being made inside the VisitorFunction, or alternatively, creating a new interface on the config which is read only and then using that inside the VisitorFunction? Either way we create a programmatic barrier to a regression if we aren't going to create tests that validate the bug/assumption?

Ah, an immutable config is a great idea. I will go ahead and work through that. Making everything underneath of the config immutable will be an effort and I am sure this will not introduce any other bugs ;)

[ivakegg]

I renig on my statement. An immutable config is NOT a great idea. The amount of code and the maintenance on that is huge. Also trying to ensure that the config is not held onto in the VisitorFunction is not doable as we use a good number of the items in the various visitors being used therein. I am not finding a good way to ensure thread safety except for using good programming practices and NOT modifying anything in the config. As it turns out there is an FSTcount AtomicBoolean which needs to be modified. So I am not all to sure where to go from here.

[ivakegg]

OK, decided on making a imutable ShardQueryConfiguration interface that can be passed through. This will at least make the programmer aware that mutating is bad.

[ivakegg]

Given the ImmutableShardQueryConfiguration interface results in so many changes, I am separating that off into a separate PR. Please see #3091 . Requesting approval of this one for expedience sake with the other one soon to follow.