Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP request reusing TCP connection leads to wrong response being processed #2506

Closed
ferencdg opened this issue Dec 7, 2020 · 4 comments · Fixed by #2507
Closed

HTTP request reusing TCP connection leads to wrong response being processed #2506

ferencdg opened this issue Dec 7, 2020 · 4 comments · Fixed by #2507

Comments

@ferencdg
Copy link
Contributor

ferencdg commented Dec 7, 2020

This issue is related to bosagora/agora#1395 (comment)

Here is how to reproduce:

interface ServiceI
{
    @safe:
    
    StructA getTest ();
    StructA getTest2 ();
}

using the clients:

code to reproduce:

setLogLevel(LogLevel.info);
auto settings = new HTTPServerSettings;
settings.port = 5000;
settings.bindAddresses = ["127.0.0.1"];
auto router = new URLRouter;
router.registerRestInterface(new Service);
auto server_addr = listenHTTP(settings, router);

runTask({
	scope (exit) exitEventLoop();
	auto settings = new RestInterfaceSettings();
	settings.httpClientSettings = new HTTPClientSettings();
	settings.httpClientSettings.readTimeout = 1.seconds;
	settings.baseURL = URL("http://127.0.0.1:5000/");
	auto service_client = new RestInterfaceClient!IService(settings);
	try {service_client.getTest(2000);} catch (Exception e) {}
	sleep(1200.msecs);
	auto result = service_client.getTest2(500);
	writeln("result:", result);
	assert(result.i == 500);
});

runApplication();

produces

[main(----) INF] Listening for requests on http://127.0.0.1:5000/
getTest receiving request with wait: 2000
getTest returning request with wait: 2000
result:TestStruct(2000)
core.exception.AssertError@source/tests.d(83): Assertion failure
----------------
??:? [0x5615a6884c05]
??:? [0x5615a68ad736]
??:? [0x5615a6890c3d]
??:? [0x5615a688479c]
/home/dan/dev/sandbox/sandbox-dlang/source/tests.d:83 [0x5615a662d4f1]
/home/dan/dev/sandbox/sandbox-dlang/../../../.dub/packages/vibe-core-1.11.1/vibe-core/source/vibe/core/task.d:712 [0x5615a666b3ac]
task.d:730 [0x5615a680a791]
task.d:439 [0x5615a6809edb]
??:? [0x5615a68869da]
[1]    171907 abort (core dumped)  ./build/app

The call to restClient.getTest2() might return the result that was intended for restClient.getTest() if the following happens:

  1. restClient.getTest() uses a timeout of 3 seconds
  2. the REST server is busy and only responds after the 3 seconds timeout
  3. restClient.getTest2() will reuse the previously opened TCP connection that belongs to the restClient.getTest() call
  4. because the serialization format of getTest1 is the same as for getTest2 the REST framework will not throw any errors
ferencdg pushed a commit to ferencdg/vibe.d that referenced this issue Dec 7, 2020
@Geod24 Geod24 changed the title major bug in TCP error handling causing REST to process wrong message HTTP request reusing TCP connection leads to wrong response being processed Dec 7, 2020
@Geod24
Copy link
Contributor

Geod24 commented Dec 7, 2020

Slightly edited the title, trying to be more precise about the causes and the consequence.
Looking at the code, it feels like the example provided is missing a critical part, the concurrency. I don't think this bug can trigger without multiple requests being sent in parallel, can it ?

@yazd
Copy link
Contributor

yazd commented Dec 7, 2020

This issue has reproducible code:
#1736

@omerfirmak
Copy link
Contributor

Slightly edited the title, trying to be more precise about the causes and the consequence.
Looking at the code, it feels like the example provided is missing a critical part, the concurrency. I don't think this bug can trigger without multiple requests being sent in parallel, can it ?

I believe it can. The concurrency just makes it easier to trigger it.

@ferencdg
Copy link
Contributor Author

ferencdg commented Dec 7, 2020

Slightly edited the title, trying to be more precise about the causes and the consequence.
Looking at the code, it feels like the example provided is missing a critical part, the concurrency. I don't think this bug can trigger without multiple requests being sent in parallel, can it ?

it can trigger without concurrency, I just updated the issue ticket with an example

ferencdg pushed a commit to ferencdg/vibe.d that referenced this issue Dec 7, 2020
ferencdg pushed a commit to ferencdg/vibe.d that referenced this issue Dec 7, 2020
gedaiu pushed a commit to gedaiu/vibe.d that referenced this issue Apr 3, 2021
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 a pull request may close this issue.

4 participants