Skip to content

Conversation

@chandu
Copy link

@chandu chandu commented Apr 6, 2014

Removed WebRequest and added classes with System.Net.Http for web communication.
Added tests for AwsRestService class.

@chandu chandu changed the title Minor refactor for Transport Minor refactor of AwsRestService Apr 6, 2014
@rexm
Copy link
Owner

rexm commented Apr 13, 2014

Thanks Chandu. There's just two issues blocking the merge into master...

  • Microsoft.Bcl and some other dependencies added to the project that are generating warnings, and it looks like the .csproj file has some extra build steps that weren't there before. This can cause some issues with cross-platform (OSX, Linux flavors, etc.) Do we need those, or can they be removed?
  • In AwsRestService.cs, I need to look closer but it looks like ExecuteRequest could cause a deadlock in some situations by calling .Result on the ExecuteRequestAsync method. Here's some background on that.. Can you let me know what you think about that?

Thanks again!

@rexm
Copy link
Owner

rexm commented Apr 13, 2014

One other note that isn't a functional issue but makes it easier to collaborate - make sure your IDE is configured to use spaces instead of tabs, and clear out any tab characters Visual Studio might've accidentally inserted.

@chandu
Copy link
Author

chandu commented Apr 14, 2014

@rexm Thanks for the feedback.

  • I will check what are the additional build steps that got added to csproj files.
  • I was not aware of the .Result resulting in a deadlock. I will read about it and see how my code can be improved to avoid deadlocks.
  • I thought I added the .editorconfig and it took care of the tabs/spaces thingy. :( . I will definitely change the files to use spaces instead of tabs.

@chandu
Copy link
Author

chandu commented Apr 21, 2014

Hi @rexm
I have done some analysis and given below are my findings:

  1. csproj files Update: I had enabled Nuget package restore while configuring the projects locally and hence the new steps in csproj file. Will get rid of the nuget restore additions.
  2. .Result resulting in a deadlock: Looked at multiple blog posts on understanding the deadlock scenario and I guess I get the gist of it. The suggested approaches were
    • either go async all the way to configure the tasks

    • to use ConfigureAwait(false) to avoid switching to the SynchronizationContext of the caller.

      I think in our case the second approach might work. The gist @ https://gist.github.com/Chandu/11148257, will depict the changes needed for this. Please let me know your comments.

  3. I will take care of tabs mixed with spaces in my subsequent commit.

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.

2 participants