Async/await support - Githubissues

axefrog commented 11 years ago

Would be good to see async/await support implemented. Doesn't matter who does it; I may get time soon(ish) but even if I don't I thought it would be worth adding it to the issues list for future development.

axefrog commented 10 years ago

Looks like I need this now. I'm going to work on it.

axefrog commented 10 years ago

I've completed an early conversion of the code to use async/await on all calls that flow through the DoRequest method, though I've had to upgrade the target platform to .Net 4.5. I'll be pushing this up in a separate branch, rather than master, as it will break most people's code. If people want to use the updated version, they'll need to use .Net 4.5, which hopefully is not a problem for most people. Changes to existing code should be mostly limited to awaiting a few calls here and there, but that should be the extent of it. I've also gotten rid of the dependencies folder, which is unnecessary and replaced it with equivalent NuGet packages. I've also updated the unit tests so that the various mocked objects cope with async methods. I'll update this ticket again after I've run some more tests and am ready to push the code up.

axefrog commented 10 years ago

I've pushed up a branch with all of the code changes that appeared to be required to support async/await. This required an upgrade to .Net 4.5. There's also a few other additions, including some extra diagnostic properties I needed but which others may also find useful, and I removed the Dependencies folder and replaced it with equivalent NuGet packages, which hopefully will make the logging component easier to work with as well. The logging component has a bug in this version; it is not capturing the raw responses correctly, so I'll need to fix this when I have time. In the mean time, feel free to test out this branch if async/await support is of any interest to you.

Teun commented 10 years ago

What does this change mean for backward compatibility? Should old code still work? I guess not, right? I guess we should discuss versioning here. We could publish a prerelease package onto NuGet and ask for feedback from the field. And maybe do some code reviewing?

axefrog commented 10 years ago

Yeah it will definitely break old code, hence the separate branch. I'm not sure what the best way to go about things is with regards to the old vs new version, I was hoping you guys could make some suggestions to that effect. Mainly I did this because I needed it, and you guys have been doing such a great job keeping the project alive and maintained that I'll happily defer to your judgement regarding where this should go. Definitely ok with code review, feel free to hit my commit and comment/question as you see fit.

Teun commented 10 years ago

I guess the way about this is like this:

your version should be labeled a prerelease by setting a prerelease version (cf SemVer) in the .nuspec file. I'd suggest 0.6.0-alpha1.
for now we'll code review and test with your code.
after that we'll publish a version to nuget, still using the prerelease version. Then we'll invite users to try it out, provide feedback
after some time (and maybe an alpha2), we'll promote to 0.6.0, merge your branch to master, making it the latest stable version. From then on, running Update-Package will ugrade most users to this version. We'll provide some explanation on how to remain on a 0.5 version (if you cannot upgrade to .NET 4.5) and what to do for upgrading. If needed, we can still make bugfixes on the 0.5 strain using a branch for that.

We could also try to keep the methods on Browser with the old signature around (Deprecated) and make them call the Task based versions, Wait() and then return the Result.

axefrog commented 10 years ago

What I've put up replaces the existing synchronous versions of the code currently, but I guess you could have sync and async versions of the code by having, say, Navigate vs NavigateAsync. The only problem is there were a couple of places in the code (e.g. where the code flows after "clicking" a link, etc.) where that approach didn't really work due to the existing legacy architecture, which means you'd end up with a library that is partially asynchronous and partially synchronous, and that seems less ideal than simply providing a legacy branch for those who want to stay there and moving other people to the async version. Everything else you said sounds fine to me. I'm not too savvy on the nuspec stuff, so I'd leave that to whoever is most comfortable doing that. Note that this whole thing is very much predicated on the idea that people actually want an async version. I haven't heard anything either way from anybody, so I'd be curious whether this is actually useful to you or anyone else currently using the component.

Teun commented 10 years ago

We could also create a set of Extension methods that implement the synchronous backward compatible behavior over the async core. Can you detail a bit on those flows where the legacy architecture prevents this?

mikaelliljedahl commented 3 years ago

Hi, I was also looking for async support. Is this fork/branch available somewhere?

kevingy commented 3 years ago

@mikaelliljedahl There is not. This work has not started.

Contributions are accepted, if you would like to fork and start it.

mikaelliljedahl commented 3 years ago

Ok the fork is ready: https://github.com/mikaelliljedahl/SimpleBrowser Addressed async/await pattern, hopefully fixed memory leaks by implementing IDisposable and moving out the Razorlight dependency to rely on DI pattern instead.

Although 29 tests of 273 are failing. I don't know if I need to set up a test environment for the unit tests because one of the failing test tries to call http://localhost/movies/ which does not exist.

kevingy commented 3 years ago

I started the work of removing external dependencies, but it turned out to be a tremendous undertaking involving rearchitecting much of the code. In doing that, I found that I was breaking more than I was fixing, so I abandoned the effort. Perhaps it's time to take up that project again, but I'd have to start from the beginning.

Much of this code is more than a decade old. Many modern software development paradigms did not exist when that code was originally written.

mikaelliljedahl commented 3 years ago

Please read my post again more carefully. Just grab my changes, the dependencies are already removed in the fork, but logging still works like before.

mikaelliljedahl commented 3 years ago

I got all tests to pass now. Will you accept my PR or do you want me to create a new Repository with the fork?

SimpleBrowserDotNet / SimpleBrowser

Async/await support #50