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

Allow using tokio's AsyncBufRead #233

Closed
wants to merge 2 commits into from
Closed

Allow using tokio's AsyncBufRead #233

wants to merge 2 commits into from

Conversation

endor
Copy link

@endor endor commented Sep 23, 2020

I would like to use quick-xml together with tokio's AsyncRead, as I am fetching an object from S3 with rusoto_s3 and that returns a body that implements AsyncRead. This is an attempt at making that happen.

@endor
Copy link
Author

endor commented Sep 24, 2020

The unit tests in unit_tests.rs and test.rs are now working with and without async.

@endor endor force-pushed the async branch 3 times, most recently from ae18715 to 5080421 Compare September 24, 2020 12:25
@endor
Copy link
Author

endor commented Sep 24, 2020

Looks like the Deserializer would need to support async as well.

@endor
Copy link
Author

endor commented Sep 24, 2020

I realize that this is not the best solution, because there's a lot of duplication. I'm just not sure how else to do it. I don't have a lot of experience with async code in Rust.

  • If we were to for example wrap a sync Reader in some kind of wrapper to make it async, so they could share more code, we would force the user to use a runtime, I guess.
  • If we were to split the async part into a separate crate/library than any changes made to one would need to manually be also made to the other.
  • Maybe we could share a bit more code if the methods that are sync in the async Reader would be put into a trait or something, but a lot of the methods are async in the async Reader and they cannot be shared.

@endor
Copy link
Author

endor commented Sep 27, 2020

After some feedback, I've changed the design a bit now and introduced an AsyncReader instead of overloading the Reader. This also allows me to leave the Deserializer as is for now and all the tests pass. There's obviously still duplication but it seems like a cleaner abstraction. The doc tests now also work.

@endor
Copy link
Author

endor commented Sep 30, 2020

@tafia Any thoughts on this?

@tafia
Copy link
Owner

tafia commented Feb 3, 2021

Ouch, it's been really long that this issue is opened, really sorry for that.
I love the idea, I need to get into it a little more and update it with (now released) tokio 1.0. Thank you so much and very sorry for the late answer.

@endor
Copy link
Author

endor commented Mar 26, 2021

I rebased the PR now on current master. Next step is to update tokio. It seems like there's a broken test in master.

@endor
Copy link
Author

endor commented Mar 29, 2021

tokio is updated.

@endor
Copy link
Author

endor commented Mar 29, 2021

I also fixed the one unit test failing in master.

@endor
Copy link
Author

endor commented Apr 19, 2021

ping @tafia

@agentsim
Copy link

@tafia Any update on whether or not this can get merged?

@tafia
Copy link
Owner

tafia commented Aug 16, 2021

Sorry, this is definitely something I consider, I'll have a look in depth today.

@agentsim
Copy link

That would be great, thanks! For what it's worth, I've been using this branch and haven't run into any problems.

@itsgreggreg
Copy link

Also FWIW, I'm writing an XMPP server and am interested in this functionality so I can switch to async multi-client handling. Happy to help push this along in any capacity. Fix conflicts, tests, etc. Lemme know if I can help!

@tafia
Copy link
Owner

tafia commented Aug 24, 2021

@itsgreggreg I'd appreciate a rebase, thanks!

@endor
Copy link
Author

endor commented Aug 24, 2021

I caught up on some of the changes that were recently made, but the BufferedInput change takes a bit more work. I might get to it soon.

@garma83
Copy link

garma83 commented May 11, 2022

any updates on this? or tips how to get this to work?

@dralley
Copy link
Collaborator

dralley commented Jul 16, 2022

Closing this old PR in favor of #417

@dralley dralley closed this Jul 16, 2022
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.

6 participants