quick-xml icon indicating copy to clipboard operation
quick-xml copied to clipboard

Add test for cancelling read_event_into_async() future

Open giorgi-o opened this issue 1 year ago • 9 comments

Addresses https://github.com/tafia/quick-xml/issues/624#issuecomment-1807077043

giorgi-o avatar Nov 13 '23 00:11 giorgi-o

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 avatar Nov 13 '23 15:11 Mingun

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

Mingun avatar Nov 22 '23 15:11 Mingun

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

Mingun avatar Nov 22 '23 15:11 Mingun

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

giorgi-o avatar Nov 25 '23 12:11 giorgi-o

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.

Mingun avatar Nov 25 '23 14:11 Mingun

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.

giorgi-o avatar Nov 26 '23 21:11 giorgi-o

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Nov 26 '23 21:11 codecov-commenter

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

giorgi-o avatar Nov 27 '23 12:11 giorgi-o

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.

Mingun avatar Nov 27 '23 14:11 Mingun