Closed gregsochanik closed 11 years ago
As an addition - this works as expected (i.e. creating a new instance within the closure)
Parallel.For(1, 10, x => Api<ArtistReleases>.Create.WithArtistId(1)
.WithPageNumber(x)
.WithPageSize(1).Please());
Max connections is not relevant. If we exceed the max connections, what will happen is that requests will get queued up and may time out before they get actioned.
What we observe with parallel loops is that some iterations silently get the wrong data - they get results that belong to a different iteration.
Greg seems to be right that the API
Creating a new API wrapper on each iteration looks like a fix, but doesn't work well with the .com website's architecture where the IFluentApi are constructor-injected. e.g.
public class TopGenresQueryHandler : IQueryHandler<TopGenresQuery, IEnumerable<Tag>>
{
private readonly IApiManager<Tags> _apiManager;
public TopGenresQueryHandler(IApiManager<Tags> apiManager)
{
_apiManager = apiManager;
}
I have the same injection issue in the sonos app, I was saying to tony today it would be nice to pinpoint exactly where the issue occurs and deal with it so as we can keep the (highly preferable) ability to inject the service.
I'll carry on looking at it tomorrow as spent all day chasing down what turned out to be a red herring :-)
As far as I can see, the problem is that API.Create() returns an IFluentApi which holds the EndPointInfo as state that is built up by following methods.
So if Thread A does wrapper.WithArtistId(1).Please() and then thread B does wrapper.WithArtistId(2).Please() they will get two different result sets. But if the threads switch over just after thread A does WithArtistId(1), the next thing is that thread B does WithArtistId(2) on the same endpointinfo - now both threads have artist id set to 2.
We want a way to give the threads separate state. And we have it, but it's at the Create() step which is 1 step too early.
It's not just the Api.Create() method, just new-ing up and sharing the state across threads of FluentApi has the same issue and as you point out (and tony pointed out to me yesterday :-)) it's a result of the shared EndPointInfo which would need to be new-ed up for each separate call.
It's a basic problem with the design, and in it's current guise in a multi-threaded set-up it can only be used if it's a completely separate instance of the FluentApi class on each thread.
Is there any point in investigating further? It may be a nice exercise to try and come up with a model in which you can share a single instance of an IFluentApi implementation within a multithreaded environment, but for now I'd just recomment that if you are to use the current FluentApi implementation within a Parallel.For, you should new it up every time :-)
Other than that - I'd like to raise the issue that we should try and tidy up the FluentApi ctor, the fact that there's so much going on there is a bit of a smell.....
It's not vital for us now. But it will probably become more important over time.
This conversation is getting a bit fragmented so I'm pulling it back into the original issue, also see pull requests #34 and #32 for further discussion.
Re @gregsochanik in #49
Using [Attributes] was a nod towards this separation, but perhaps we need to go further and set what we consider >immutable properties at set up, then a consumer is forced to create a new FluentApi if, for example, the HttpMethod >they are using to hit an endpoint changes?
Yeah, that's exactly what I've been thinking recently, although my ideas need some more work and verification. I agree we need to be more explicit about the separation of the two concepts of endpoint configuration on FluentApi<>
instantiation and building up the request parameters using the chained fluent methods. The RequestData
has ended up being the representation both of these things.
I think we should try splitting the RequestData
into two:
EndpointContext
containing the invariant info which is populated at instantiation from the meta data.RequestBuilder
databag to hold the properties of the individual request which gets populated by the WithXXX
and UseXXX
helpers before being turned into an IRequest for dispatching.If our WithXXX()
, UseXXX()
methods tranisitioned to a different interface from IFluentApi<>
(such as IApiRequest<>
) as @AnthonySteele suggested then the IFluentApi<>
might never need to know about the new RequestBuilder
- each thread would get it's own one as it would be owned by the IApiRequest<>
. The final call to Please()
could then call back into the original IFluentApi<>
that created the IApiRequest<>
instance giving it the RequestBuilder
, but so long as the IFluentApi<>
never holds a reference to the IApiRequest<>
or RequestBuilder
there should be no mutable shared state.
The downsides of course are:
WithXXX()
UseXXX()
methods to return the new IApiRequest<>
methods on both IFluentApi<>
and IApiRequest<>
UsingClient
- should that be shared or per-request?IApiRequest<>
and shares it between threads, bad things will still happen.I'll knock up a spike when I'm back on a windows machine.
A potentially showstopping impact of this approach is the requirement to force a transition to IApiRequest<>
from IFluentApi<>
before using any of the extension methods, I guess this could be done with a MakeRequest()
method on IFluentApi<>
. That change will effectively break all callsites (but at least the interface remains the same for both parrallel and normal usage):
var keane = Api<Artist>.Create.MakeRequest().WithArtistId(1).Please();
or
var artistReleases = new FluentApi<ArtistReleases>();
Parallel.For(1, 10, x => artistReleases
.MakeRequest()
.WithArtistId(1)
.WithPageNumber(x)
.WithPageSize(1)
.Please());
I suppose we could duplicate all the extension methods to extend both IFluentApi<>
and IApiRequest<>
(and always return an IApiRequest<>
) but that would be a pain to maintain going forward.
Back to square one?
I agree with your thinking here. A breaking MakeRequest() method seems useful.
:+1:
There are some other (hopeefully minor) ramifications that need some thought. I knocked up a spike on my laptop over the weekend, but I havent pushed it. I'll do so this evening, so that we've gott something concrete to discuss / play with.
I've noticed (via wireshark) that if you try and request using the ApiWrapper in parallel:
e.g.
in an attempt to load many results at the same time, then on occasions, the ApiWrapper makes multiple calls using the same query string parameters. In the above case, the pageNumber is the same for 2+ separate queries. Its occurs with varying frequency which leads me to believe that it's a concurrency issue.
I will do some digging and see what the culprit is.