Skip to content

feat!: make read_only_data_batch truly const - #192

Open
dhruv9vats wants to merge 5 commits into
mainfrom
make-read-only-const
Open

dhruv9vats wants to merge 5 commits into
mainfrom
make-read-only-const

Conversation

@dhruv9vats

Copy link
Copy Markdown
Member

Make read_only_data_batch truly const, disallowing any mutating operations, directly or indirectly.

@copy-pr-bot

copy-pr-bot Bot commented Aug 19, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@dhruv9vats
dhruv9vats marked this pull request as ready for review August 19, 2026 14:02
@dhruv9vats

Copy link
Copy Markdown
Member Author

The failing test is a flaky one, because:

  • The main thread acquires ro_initial, but t1 releases it; unlocking a mutex from another thread is undefined behavior.
  • The 50 ms sleep does not guarantee that t2 attempted or blocked on mutable acquisition.
  • The last reader can unlock before t1 logs its release, allowing t2 to log acquisition first and fail the ordering assertion. (This is the cause of the seen failure)
  • After thread2_mutable_released is set, t1 can acquire new readers before t2 asserts idle.

So disabling the test for now.

@9prady9 9prady9 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

whats the contract we are trying to adhere here ? for the readonly_to_mutable and mutable_to_readonly . Later breaks the API, former make it strict read-only. They are not exactly illegal is my question about.

the const ness change looks good.

@dhruv9vats

Copy link
Copy Markdown
Member Author

These changes basically lay the groundwork for sirius-db/sirius#1063 but incrementally molding the API. The removal of the conversion API basically mandates that the user go through the idle state before acquiring the alternate state. Going from idle to mutable was a grey area anyways because that meant that anyone with a read only handle could get to and mutate the internal state, which broke the invariant of immutability.

*
* Atomic, lock-free.
*/
void subscribe();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It is a bit strange but I believe these now need to be const. E.g., if a read only data batch is the only owner, and then transitions to idle, it cannot unsubscribe (compile error). Worth adding a test for this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants