Add allowParallel flag to allow multiple readers to be created

[nh2]

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

https://github.com/davidcaron/pye57/commit/7d2dd716b5574063559e1fc7d355355ed68f38a4

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 https://github.com/asmaloney/libE57Format/pull/292#issuecomment-2181565633, 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.