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.

Sign up for GitHub

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

Jump to bottom

Refactor tag assertion to FatalError check & Fix unexpected increase of tag #231

Closed
wants to merge 5 commits into from
Closed

Refactor tag assertion to FatalError check & Fix unexpected increase of tag #231

wants to merge 5 commits into from

Conversation

qsdgy
Copy link

@qsdgy qsdgy commented Jul 4, 2022 •
edited by jonhoo
Loading

Try to turn tag assertion to an error check #230
Also straight forward fix #229

This change is Reviewable

@jonhoo
Copy link
Owner

jonhoo commented Jul 4, 2022

Thanks for taking this on! I don't think this should be a separate "special" error type — many of the other existing error variants are just as fatal as this one. I think I'd be fine with just adding an error variant for tag mismatch and return that as an error like any other instead of panicking.

@codecov
Copy link

codecov bot commented Jul 4, 2022 •
edited
Loading

Codecov Report

Merging #231 (3ca8527) into master (8d031f2) will decrease coverage by 1.17%.
The diff coverage is 16.66%.

❗ Current head 3ca8527 differs from pull request most recent head 6886d4d. Consider uploading reports for the commit 6886d4d to get more accurate results

Impacted Files Coverage Δ
src/error.rs 12.94% <0.00%> (-1.35%) ⬇️
src/client.rs 83.13% <30.00%> (-0.83%) ⬇️
src/client_builder.rs 27.27% <0.00%> (-8.73%) ⬇️
src/parse.rs 84.13% <0.00%> (-1.33%) ⬇️
src/types/deleted.rs 97.22% <0.00%> (ø)
src/types/appended.rs
src/types/fetch.rs 41.42% <0.00%> (+2.23%) ⬆️

@qsdgy
Copy link
Author

qsdgy commented Jul 5, 2022

It's ok to flatten it.

Is it possible that we specify out what are fatal / resumable / determines errors ?
We could indicate it by doc / error type / fn on error / fn on session , providing more flexibility. Restart seesion on any error seems a bit wild.

jonhoo
jonhoo requested changes Aug 13, 2022
View reviewed changes
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Sorry it took me so long!

src/client.rs
self.write_line(command.into_bytes().as_slice())
let result = self.write_line(command.into_bytes().as_slice());
if result.is_err() {
self.tag -= 1; // rollback tag increase in create_command()
Copy link
Owner

Choose a reason for hiding this comment

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

Following the discussion in #229, I'm okay with this change, though I think we should also add a skip_tag method to Client to allow manually incrementing the tag.

src/error.rs
@@ -104,6 +104,8 @@ pub enum Error {
/// In response to a STATUS command, the server sent OK without actually sending any STATUS
/// responses first.
MissingStatusResponse,
/// Tag mismatch between client and server. New session must be created.
TagCorrupted(TagCorrupted),
Copy link
Owner

Choose a reason for hiding this comment

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

I think the name TagCorrupted is misleading. A "corrupted" tag feels more like if a tag isn't a number for example, or has somehow otherwise been modified in-flight. What this represents (as the doc string indicates) is a TagMismatch or Desynchronized, so let's call it one of those.

src/error.rs
Comment on lines +314 to +320
fn description(&self) -> &str {
"Tag is corrupted, session is in an inconsistent state"
}

fn cause(&self) -> Option<&dyn StdError> {
None
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think you can skip both of these fns — they're not required and their default implementations will do the right thing.

@soywod
Copy link
Contributor

soywod commented Jun 6, 2023

Is there anything I can do to to help this PR to be merged? I also encounter the same issue when retrying a .authenticate() after an error (refreshing an expired OAuth 2.0 access token):

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `[97, 49]`,
 right: `[97, 50]`', ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/imap-3.0.0-alpha.10/src/client.rs:1538:25
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@jonhoo
Copy link
Owner

jonhoo commented Aug 13, 2023

I think my comments above still stand, so it's really just a matter of making those changes. If @qsdgy is still interested in picking that up, then that's probably easiest, but if they're otherwise preoccupied or no longer need this, then feel free to open a new PR that builds on this one so we can land it 👍

soywod added a commit to soywod/rust-imap that referenced this pull request Mar 24, 2024
@soywod
Apply PR jonhoo#231
5ed1f39
@soywod soywod mentioned this pull request Mar 24, 2024
Merged
@soywod
Copy link
Contributor

soywod commented Mar 24, 2024

then feel free to open a new PR that builds on this one so we can land it 👍

I opened a new PR and tried to apply the changes your were requesting: #284

@jonhoo
Copy link
Owner

jonhoo commented Mar 30, 2024

Closing this in favor of #284 👍

@jonhoo jonhoo closed this Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonhoo jonhoo jonhoo requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unexpected increase of imap::client::Connection.tag
3 participants
@qsdgy @jonhoo @soywod