search

kenakamu / UCWA2.0-CS

C# library for UCWA 2.0
MIT License
24 stars 13 forks source link

Multiple UCWAClient instances #45

Closed btastic closed 6 years ago

btastic commented 6 years ago

Hey there,

I tried having multiple UCWAClient instances on the same SfB Host with different logins. However, the HttpClient pooling gives every UCWAClient the same HttpClient (pooling per hostname). Now I receive the same events for different clients and don't know how to react to each of them. I implemented something like an identifier for each HttpMethod a client can use. It was tedious, but it is working.

However, is there maybe a more simple approach to that?

Another question: The main GetEvent loop is running without it really being cancellable. When I SignOut and dispose everything, some calls might still be stuck within the GetEvent loop, leading to exceptions Maybe we can implement CancellationTokens? Any thoughts on that?

Thanks!

baywet commented 6 years ago

Hi, I believe the mess around multiple UCWAClient mixing their HttpClient (and events and so on) is caused because the HttpService is all static. I wanted to transition away from that because of this issue and also because it doesn't allow to do any dependency injection (which would make unit testing with mocking easier). For the cancellation token I agree it'd be a nice to have, however it's a deep change because we need to pass it around in all layers. And if we don't want to break compatibility, it should be an optional last parameter for the higher level methods (the ones people use).

I think those are two separate issues, do you mind splitting those? Also do you think you can author separate pull requests for those two?

btastic commented 6 years ago

Hi,

what do you have in mind by transition away from the all static HttpService? Who is owning the HttpClient and keeping it? The most confusing thing to architect is moving the Http calls inside the Model classes. I don't know an elegant way of changing that. Looking back, my current solution is kind of messy and I don't like it at all. Do you mind giving me a push in direction you are thinking?

baywet commented 6 years ago

https://github.com/kenakamu/UCWA2.0-CS/blob/master/UCWASDK/UCWASDK/Services/HttpService.cs All of that is static. As a result any instance of UCWAClient in the app domain is referencing the same unique instance. And the pool of HttpClient is the same for everybody. (same for this https://github.com/kenakamu/UCWA2.0-CS/blob/master/UCWASDK/UCWASDK/Services/Settings.cs and that https://github.com/kenakamu/UCWA2.0-CS/blob/master/UCWASDK/UCWASDK/UCWAClient.cs#L12-L13) Do you think you can author a PR for that as well? if you author a PR for both, please make it 2 separate PR's.

btastic commented 6 years ago

Yes I could do it, if you'd give me some advice on how to architect the wanted solution.

baywet commented 6 years ago

Basically remove all the static keywords from the library. Then the UCWAClient should have a private property of HttpService initialized either by property initialization or by constructor. The trick is: it's methods are public, I know I'm not using it in my projects but other might be to manually craft requests: either we have to provide some compatibility or document that as a breaking change. I'm curious to know how many are actually using that class.

For Settings, the idea is the same but it's a bit trickier, people are more likely to use it outside of the library, I think we should provide some compatibility mechanism. And it needs to be passed downed a couple of places.

For the events I'm not sure yet what should be the approach.

btastic commented 6 years ago

Unfortunately the project in which I was using your library was canceled internally. So I am no longer working on this.

baywet commented 6 years ago

Hum that's too bad. Ok thanks for reporting the issues, we'll see if we can tackle it.

From: Ben Konsemüller notifications@github.com Sent: Friday, February 2, 2018 4:44:50 AM To: kenakamu/UCWA2.0-CS Cc: Vincent Biret; Assign Subject: Re: [kenakamu/UCWA2.0-CS] Multiple UCWAClient instances (#45)

Unfortunately the project in which I was using your library was canceled internally. So I am no longer working on this.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHubhttps://github.com/kenakamu/UCWA2.0-CS/issues/45#issuecomment-362537167, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AHig3jxds_AAoP97verAFAbAp0ADNTG0ks5tQtkSgaJpZM4Rlzp9.

kenakamu commented 6 years ago

Do we really need this? If we work on server side app, yes but this is more like for client side app? I close this for now.

baywet commented 6 years ago

In my case we're using the library in a server side context because we built an API for front end components. As the library can be used in both contexts and this is a design issue we should probably let it open and discuss about a solution