Upgrade Http::Stream::reply to Pointer

[yadij]

Refactor to avoid storing a raw-pointer, with
missing reference-counting.

[kinkie]

This PR seems to contain spurious changes

[rousskov]

Thank you for this upgrade.

I replaced "update" with "upgrade" in PR title because the latter feels more precise and arguably self-documents the motivation behind this PR. Please undo if you think "update" is better.

[rousskov]

If possible:

Suggested change

	MemBuf *mb = reply->pack();
	const auto mb = reply->pack();

Otherwise:

Suggested change

	MemBuf *mb = reply->pack();
	auto mb = reply->pack();

[yadij]

mb gets appended to below, going with the non-const variant.

[rousskov]

mb gets appended to below, going with the non-const variant.

mb is not appended to. *mb is appended to. The change request was formulated correctly and still stands.

[rousskov]

This HttpReply object may be passed from its raw/unprotected/lockless DeferredParams::reply storage to now-refconted Http::Stream::reply storage. I hope those deferred parameters always belong to the same Http::Stream object, but if they are not, then Http::Stream::reply object might be destroyed (via now-added Http::Stream::reply reference counting) before the corresponding DeferredParams object is destroyed, creating a dangling pointer inside DeferredParams storage. I recommend upgrading Http::Stream::DeferredParams::reply together with Http::Stream::reply (i.e. in this PR) to mitigate that risk.

[rousskov]

Looking at branch commit 7516249 (and, to a degree, its parent), it feels like something went wrong while upgrading Http::Stream::DeferredParams::reply storage. If possible, please avoid changing all (or the vast majority of) those functions! During these transitions, it is OK to pass a raw pointer (to a reference-counted HttpReply object) to legacy code that does not store that pointer or stores it properly. AFAICT, most of those modified functions fall into the former category. They should not be modified.

If there are some problems with that approach, let's discuss.

[yadij]

There is a chain reaction going on here. This is as small a chain of side effects as I can make it.

A) To fix the dangling pointer in Http::Stream::DeferredParams::reply update Http::Stream::DeferredParams::reply to RefCount.

That pushes the dangling pointer problem into ESIIncludeContext and Downloader where class methods using CSCB type for AsyncCall parameters as a raw-pointer (now the dangling one).

B) To fix (A) update CSCB definition to pass RefCount instead. Which pulls in all the CSCB functions.

[rousskov]

There is a chain reaction going on here. This is as small a chain of side effects as I can make it.

Thank you for detailing this! I hope we can find a way to stop that chain reaction much sooner than branch commit 7516249 does:

A) To fix the dangling pointer in Http::Stream::DeferredParams::reply update Http::Stream::DeferredParams::reply to RefCount.

That pushes the dangling pointer problem into ESIIncludeContext and Downloader where class methods using CSCB type for AsyncCall parameters as a raw-pointer (now the dangling one).

CSCB callbacks are synchronous, right? When I look at Downloader changes in branch commit 7516249, I do not see any long-term HttpReply storage. I only see caller's reply pointer being forwarded to another synchronous call. That kind of forwarding, by itself, does not require smart pointers and does not create dangling raw pointers. Let's temporary assume, to simplify arguments, that downloaderRecipient() is the only CSCB callback in existence. Why do we have to change downloaderRecipient() and CSCB type after changing Http::Stream::DeferredParams::reply?