Skip to content

Conversation

@Mpdreamz
Copy link
Member

Everything old is new again, some lessons have to be learned over and over 😢

In NEST 0.x and 1.x the HttpConnection async bits is implemented using a marvalous APM callback soup implementation which was hard to read but battle hardened.

With 2.x we finally took the plunge to a TAP and async/await based implementation which is a joy to read. However in doing so we did not port all the timeouts registered with the threadpool to make the unabortable bits such as GetRequestStreamAsync and GetResponseAsync.

This PR brings back some APM wrapped to a Task with Task.Factory.FromAsync and ThreadPool.RegisterWaitForSingleObject is back in action to guard these calls.

To test this works start an 5.x elasticsearch server and use the AutoResponder in fiddler and set up the following rules:

image

Then using

static void Main(string[] args)
{
	Task.Run(async () =>
	{
		var connectionSettings = new ConnectionSettings(new Uri("http://ipv4.fiddler:9200"))
			.RequestTimeout(TimeSpan.FromSeconds(3));
		var client = new ElasticClient(connectionSettings);

		//var response = client.RootNodeInfo();
		var response = await client.RootNodeInfoAsync();
		Console.WriteLine(response.DebugInformation);
		Console.ReadKey();
	}).Wait();
}

First using the latest rc3 you'll see the synchronous code path adheres the timeout but the async code path does not.

If you then add manual references to a build from this branch you'll see both behaving fine.

If you uncheck the delay rule in fiddler you'll see both request succeeding again with no timeouts.

@russcam
Copy link
Contributor

russcam commented Nov 30, 2016

LGTM 👍 will merge into 5.x and port to 2.x and master now

@russcam russcam force-pushed the fix/http-connection-unabortable-bits branch from d4676ae to c6a20e8 Compare December 1, 2016 00:02
@russcam russcam merged commit c6a20e8 into 5.x Dec 1, 2016
@russcam
Copy link
Contributor

russcam commented Dec 1, 2016

@Mpdreamz Mpdreamz deleted the fix/http-connection-unabortable-bits branch January 23, 2017 21:08
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.

3 participants