Wt

Hello,

Do you have an indication on when this happens? Can you share a test case?

Wim.

Hi Wim,

So in my usage, its not reproducible every time, hence I've resorted to looping enough times to trigger the issue.

So https://redmine.webtoolkit.eu/issues/5903 has the bug report.

https://redmine.webtoolkit.eu/attachments/download/2608/IntegrationTestHelper.hpp wraps the Integration test HttpClientServer test from the Wt repo with some helper functionality.

I think this is a race condition within Wt::Http::Client::Impl, related to cancellation of outstanding requests vs timeouts. see (1)

The test should stream a vector of strings using Wt::WResponseContinuation, this does complete successfully with the attached test case.

The abort case, as modelled with Test::Client::abortAfterHeaders() which does result in a call of Wt::Http::Client::abort() from within Test::Client::onHeadersReceived() is

https://redmine.webtoolkit.eu/attachments/2607/test_http_streaming_slow.cpp which runs enough iterations to trigger the bug.

(1) my half backed guess

Wt::Http::Client::abort() branches on Wt::Http::Client::Impl::hasServer() - confusingly as the behaviour is identical on both sides of the branch,

calls Wt::Http::Client::Impl::asyncStop() then boost::shared_ptr::reset()

Client::Impl::asyncStop() will call shutdown then close on the socket - the close cancelling outstanding asio operations I think.

Client::Impl::timeout() will call shutdown but not close on the socket so not cancelling the operations, and leading to the results from the test case;

Jamal

Jamal

Hi Wim,

Following up, I tried the following patch to see if my guess was correct, and it is not - so I'm somewhat confused now - any ideas on how to help you guys diagnose this?

Hi,

When I test your test case it just seems to hang because a request that is aborted is never done?

Regards,

Roel

Hi Roel,

After stop() [2] is called, the underlying socket has been shutdown so the request is done/dead.

Client::abort()[4] basically is complete() [3] but without something like emitDone(boost::asio::error::operation_aborted(), response_) to notify the waiter.

The API docs seems to suggest the test case / supporting code can expect onDone even on error / abort [1]

So is this a bug, or is my code broken and how do you suggest is best to modify the code to not hang.

Regards

Jamal

[1] https://www.webtoolkit.eu/wt/doc/reference/html/classWt_1_1Http_1_1Client.html#ad288d9121ba7d6f7dd1e29c9ed2a627d

[2] https://github.com/emweb/wt/blob/master/src/Wt/Http/Client.C#L151

[3] https://github.com/emweb/wt/blob/master/src/Wt/Http/Client.C#L574

[4] https://github.com/emweb/wt/blob/master/src/Wt/Http/Client.C#L789

After looking into it a bit further, it seems that you are assuming that the resource will always end up in the handleAbort case if the request is cancelled by the client. I don't think it's possible to guarantee that. The server may have already sent off all of the data before the operation is aborted.

I still have to dig into this emitting of Client::done(), if that should definitely happen on abort. As it is right now, the documentation can be interpreted in two ways:

1. When the client is aborted, done() will always be emitted with an error code (which we neglected to specify and we should also fix that)
2. When the client is aborted, if done() is emitted, it will be emitted with an error code, but there's no promise that it will always be emitted

I'm not sure why you think there is a possible leak, though. I don't see a possibility for that.

Hi,

In the test case we abort after headers, so should always be more data to send..

1) is the only sane case, otherwise you have no way to wait for completion or error of the request.

2) leads to the current thread hanging waiting for a unlock that will never come and it's weird semantically, your request is dead, it's "done".

Also the underlying behaviour is not deterministic, it should either always work or always fail.

When it succeeds, we should always get all the data, and when it fails we should be notified. These invariants don't seem to always be maintained.

It's possible to leak as resources allocated for the request for example the Client::Impl are not released until done() is called, which is also tying up a thread, eventually we'll run out of threads and not serve any more clients.

btw when is cppcon going to post your talk, slides are up but no video..

It's always possible that the response is sent properly on the server side, while it is aborted client side, so the count of errors does not have to be the same. Usually, in your test case, the client will have more failures than the server.

I did go through the code of the client and made sure that done() is always emitted exactly once per request. I just pushed those changes to GitHub.

Regards,

Roel

Hi Roel,

That's great news, thank you for looking into it.

So I don't quite understand this, 'It's always possible that the response is sent properly on the server side, while it is aborted client side, so the count of errors does not have to be the same. Usually, in your testcase, the client will have more failures than the server.'

In my testcase the server errors are instances of handleAbort() being called, and every request calls abort after writing headers, so (resets+failures i server_errors()) should hold?

Regards

Jamal

On the server side, handleAbort will only be called if there's an outstanding continuation, in order to perform some cleanup.

In your test case the client initiates an abort after receiving the headers. However, the client may have already started listening for more data before you abort it. The server may then have already continued to process this request (it has already reentered handleRequest). If there is no continuation anymore, the server already knows it can perform some cleanup, and handleAbort does not have to be called anymore. In that case the client may consider the request aborted, even though handleAbort is not called on the server side.

Regards,

Roel

Hi Roel,

Thanks again for looking through this.

So can I assume that if I've passed a continuation that handleAbort will always be called to allow me to clean up.

For example.

I'm using packaged_task for clarity below but if understand you correctly, the following code should be safe from leaks.

Regards

Jamal

struct Foo

{

std::futurestd::string future_result;

};

void handleRequest(const Wt::Http::Request& request, Wt::Http::Response& response)

{

auto c = request.continuation();

if(!c)

{

c = response.createContinuation();

std::packaged_task<std::string()> task(

[&c](){

// do something that blocks e.g. make http request to backend

continuation->hasMoreData();

return "future hello";

});

// create a source of leakage to demonstrate the point.

Foo * foo_ptr = new Foo;

foo_ptr->future_result = task.get_future();

c->setData(foo_ptr);

c->waitForMoreData();

}

else

{

// request is done write result, cleaning up resource

auto foo_ptr = std::any_cast<Foo *>(c->data());

response.out() << foo_ptr->future_result.get();

delete foo_ptr;

foo_ptr = nullptr;

}

}

void handleAbort(const Http::Request& request)

{

auto c = response.continuation();

if©

{

// we should always be called to cleanup

auto foo_ptr = std::any_cast<Foo *>(c->data());

delete foo_ptr;

foo_ptr = nullptr;

}

}

That should indeed be safe.

Regards,

Roel

Thanks again Roel,

We can close this issue now.