Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use explicit threads. #65

Merged
merged 2 commits into from
Apr 26, 2021
Merged

Use explicit threads. #65

merged 2 commits into from
Apr 26, 2021

Conversation

hlinnaka
Copy link
Contributor

Remove 'async' usage a much as feasible. Async code is harder to debug,
and mixing async and non-async code is a recipe for confusion and bugs.

There are a couple of exceptions:

  • The code in walredo.rs, which needs to read and write to the child
    process simultaneously, still uses async. It's more convenient there.
    The 'async' usage is carefully limited to just the functions that
    communicate with the child process.

  • Code in walreceiver.rs that uses tokio-postgres to do streaming
    replication. We have to use async there, because tokio-postgres is
    async. Most rust-postgres functionality has non-async wrappers, but
    not the new replication client code. The async usage is very limited
    here, too: we use just block_on to call the tokio-postgres functions.

The code in 'page_service.rs' now launches a dedicated thread for each
connection.

This replaces tokio::sync::watch::channel with std::sync:mpsc in
'seqwait.rs', to make that non-async. It's not a drop-in replacement,
though: std::sync::mpsc doesn't support multiple consumers, so we cannot
share a channel between multiple waiters. So this removes the code to
check if an existing channel can be reused, and creates a new one for
each waiter. That created another problem: BTreeMap cannot hold
duplicates, so I replaced that with BinaryHeap.

Similarly, the tokio::{mpsc, oneshot} channels used between WAL redo
manager and PageCache are replaced with std::sync::mpsc. (There is no
separate 'oneshot' channel in the standard library.)

See discussion at #58

@ericseppanen
Copy link
Contributor

Can we make a copy of the original SeqWait, perhaps save it as zenith_utils::seqwait_async::SeqWait?

@ericseppanen
Copy link
Contributor

The new SeqWait looks good to me. It's pretty cool how the unit tests have almost no diff at all :)

@hlinnaka hlinnaka force-pushed the refactor-no-async branch 6 times, most recently from 9c9b12c to 595764b Compare April 26, 2021 08:14
@hlinnaka
Copy link
Contributor Author

Rebased, and saved a copy of async SeqWait per Eric's suggestion.

@hlinnaka hlinnaka force-pushed the refactor-no-async branch 2 times, most recently from 39f61c3 to cb47989 Compare April 26, 2021 10:05
Remove 'async' usage a much as feasible. Async code is harder to debug,
and mixing async and non-async code is a recipe for confusion and bugs.

There are a couple of exceptions:

- The code in walredo.rs, which needs to read and write to the child
  process simultaneously, still uses async. It's more convenient there.
  The 'async' usage is carefully limited to just the functions that
  communicate with the child process.

- Code in walreceiver.rs that uses tokio-postgres to do streaming
  replication. We have to use async there, because tokio-postgres is
  async. Most rust-postgres functionality has non-async wrappers, but
  not the new replication client code. The async usage is very limited
  here, too: we use just block_on to call the tokio-postgres functions.

The code in 'page_service.rs' now launches a dedicated thread for each
connection.

This replaces tokio::sync::watch::channel with std::sync:mpsc in
'seqwait.rs', to make that non-async. It's not a drop-in replacement,
though: std::sync::mpsc doesn't support multiple consumers, so we cannot
share a channel between multiple waiters. So this removes the code to
check if an existing channel can be reused, and creates a new one for
each waiter. That created another problem: BTreeMap cannot hold
duplicates, so I replaced that with BinaryHeap.

Similarly, the tokio::{mpsc, oneshot} channels used between WAL redo
manager and PageCache are replaced with std::sync::mpsc. (There is no
separate 'oneshot' channel in the standard library.)

Fixes github issue #58, and coincidentally also issue #66.
@hlinnaka hlinnaka force-pushed the refactor-no-async branch from cb47989 to 098ce26 Compare April 26, 2021 10:09
It is currently unused, and is not built as part of 'cargo build', but
seems like a shame to throw it away completely.
@hlinnaka hlinnaka force-pushed the refactor-no-async branch from 098ce26 to bc652e9 Compare April 26, 2021 10:30
@hlinnaka hlinnaka merged commit bc652e9 into main Apr 26, 2021
@hlinnaka hlinnaka deleted the refactor-no-async branch April 26, 2021 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants