Add allowParallel flag to allow multiple readers to be created

[nh2]

Handy for multithreaded work, we also use it in an extension of pye57:

davidcaron/pye57@7d2dd71

CC @chpatrick

[asmaloney]

Thanks Niklas.

This library wasn't designed to handle anything parallel/threaded. That's probably why they put that check in there.

So I'm reticent to add a "skip this error" flag that would imply that it's supported in any way....

[chpatrick]

Hi Andy, I think that's totally sensible, but our use case is completely read-only, and I think in that case it makes sense to opt into multi threading at your own risk.

…

On Thu, Jun 20, 2024, 23:08 Andy Maloney ***@***.***> wrote: Thanks Niklas. This library wasn't designed to handle *anything* parallel/threaded. That's probably why they put that check in there. So I'm *reticent* to add a "skip this error" flag that would imply that it's supported in any way.... — Reply to this email directly, view it on GitHub <#292 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGLJTZI5XY7AIBMVRATBHLZINAGNAVCNFSM6AAAAABJM2JE5OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBRGU3DKNRTGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[nh2]

Right, in the same way as any other C++ data structure allows, e.g. std::vector. It allows parallel reading even though .push_back() is completely thread-unsafe.

The way I see it, your library provides a way to prevent parallel access (better than std::vector in this regard which has no saferails). But there should be a way to turn that off for read-only use cases.

[asmaloney]

After thinking about it a bit, this feels more like a workaround than a proper solution.

Some of the code in this library is kinda... old & gnarly. To feel more comfortable with moving in this direction, I'd like to see:

• a test added to the test suite doing CV reads on multiple threads
• a test using multiple threads with the "simple" API reader (probably run into this)
• the CI fixed for the two cases that are broken so we know this works across all tested platforms

Then with the tests in place:

• fix any other issues that come up
• remove the readerCount() check in CompressedVectorNodeImpl completely

Then the lib would fully support multiple readers.

[nigels-com]

Ouch. We have a backlogged ticket for testing multi-threaded reading and writing of seperate point clouds.
We may be able to contribute to this effort, at least by running it though some internal test coverage.