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

Add test for cancelling read_event_into_async() future #680

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

giorgi-o
Copy link

Addresses #624 (comment)

@Mingun
Copy link
Collaborator

Mingun commented Nov 13, 2023

Thanks for PR! It is useful. I (or someone else who is interested) use it when I'll deal with the problem, but for now let it hang open.

@Mingun
Copy link
Collaborator

Mingun commented Nov 22, 2023

Probably this bug is OS-dependent. I tried to increase for loops to 0..3000 and didn't get errors on Windows 11

@Mingun
Copy link
Collaborator

Mingun commented Nov 22, 2023

In case if GitHub delete logs -- error is

failures:

---- test_cancel_future stdout ----
thread 'test_cancel_future' panicked at tests/async-tokio.rs:79:22:
called `Result::unwrap()` on an `Err` value: IllFormed(MismatchedEnd { expected: "<tag", found: "tag" })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@giorgi-o
Copy link
Author

Note that I am also running Windows 11, and the error occurs consistently on my machine.

@Mingun
Copy link
Collaborator

Mingun commented Nov 25, 2023

Well, then I can say, that you probably should try to debug and fix it yourself. I'll plan to make a PR with completely new parser soon, which maybe will free of this error, so you can wait until that.

Because you already did some debugging, maybe you can rewrite the test to be more straightforward, without loops, just with manual calls in sequence. I think that currently the test unstable because it rely on system timer.

@giorgi-o
Copy link
Author

I refactored it to be more deterministic, removing the arbitrary loop and replacing the 1ms timeout with 100ms.

As for fixing the issue myself, I'll see if I can make time to get familiar with the codebase, although I unfortunately doubt I can in the next few weeks. Hopefully the new parser addresses the problem.

@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bbc7bda) 65.16% compared to head (2971387) 65.53%.
Report is 28 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #680      +/-   ##
==========================================
+ Coverage   65.16%   65.53%   +0.36%     
==========================================
  Files          38       38              
  Lines       17851    18027     +176     
==========================================
+ Hits        11633    11814     +181     
+ Misses       6218     6213       -5     
Flag Coverage Δ
unittests 65.53% <ø> (+0.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@giorgi-o
Copy link
Author

And now it crashes on my machine but not on CI... I'll have to take a closer look

@Mingun
Copy link
Collaborator

Mingun commented Nov 27, 2023

replacing the 1ms timeout with 100ms.

Probably the problem is more visible for small timeouts rather than big. Maybe this is even a bug in tokio, because it seems some data race which should be impossible in safe Rust. Because quick-xml does not use unsafe, the error should be in the tokio runtime. Although I'm not a specialist in async code, so this just my speculations.

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.

None yet

3 participants