Stream responses where appropriate

[yhabteab]

This PR changes some of our HTTP handlers to stream responses instead of buffering them in memory, just like how @jschmidt-icinga did for the ObjectQueryHandler in #10516.

fixes #10544
fixes #10407

[jschmidt-icinga]

Looks good. The change to return http::status::accepted instead seems reasonable, but shouldn't that be documented somewhere?

I didn't find much referencing the status codes to expect from our API, except the example client code for the various languages. At least those should be adapted. Maybe adding a small paragraph/table detailing which handlers return 200 and which return 202 would be a good idea.

Edit: The "Responses" section needs to be adapted.

[yhabteab]

The change to return http::status::accepted instead seems reasonable, but shouldn't that be documented somewhere?

Not that I know of. Of course, this is kinda a breaking change, so it should definitely be mentioned in the changelog, but I don't think that we have a specific place for documenting changes to the API return codes.

Edit: The "Responses" section needs to be adapted.

Ok!

[jschmidt-icinga]

Edit: The "Responses" section needs to be adapted.

On second thought, I think it is basically fine as it is.

The next section "HTTP Statuses" mentions that everything from 200 to 299 is a success. Maybe a short mention that most handlers return either 200 or 202 in case of success, depending on whether the result is known at the time the Header is sent.

But the example code snippets definitely need to be fixed, currently they all check for Ok/200 specifically on "v1/objects/services", which would fail now. Even though that's easy to notice what's wrong, that'd not be a good documentation experience for the user.

[jschmidt-icinga]

I haven't verified the changes to the golang snippet, but otherwise the documentation changes look great. I also gave each handler a quick test yesterday and everything works as expected.

After looking through the remaining handlers, another one that could be streamed is /v1/templates. The change would be minimal, I think freezing the Array and adding the yc to SendJsonBody() should be enough now. Same for the GET side of /v1/config/packages and /v1/config/stages. Those are probably not strictly necessary, but I'd say about on the same level as /v1/types and /v1/variables and since you did those, you might as well do these.

[yhabteab]

Same for the GET side of /v1/config/packages

That handler holds a locked mutex while generating the body, so can't use streaming. Plus, the response is minimal, so it doesn't need any workaround.

[yhabteab]

The change would be minimal, I think freezing the Array and adding the yc to SendJsonBody() should be enough now. Same for the GET side of /v1/config/packages and /v1/config/stages.

Done! I've frozen the results array for the /v1/config/packages handler as well, to allow streaming in case the package has a bunch of orphaned stages that could exceed our flush threshold when accumulated.

[jschmidt-icinga]

Does one of you understand why this test failed? It says AssertServerDisconnected was unsuccessful. I'm generally not especially happy with that code, but it shouldn't cause any problems, 11 seconds is generous as it is. Also this PR touches none of that code and I haven't seen a check fail like that until now.

The exception could be from the HttpServerConnection object outliving the stream/socket object due to the keep_alive in the coroutine. But what is preventing CheckLiveness form initiating the disconnect here?

Edit: It might be because of a clock anomaly where the runner might have adjusted time back a second which would affect HttpServerConnection, but not the test case. Fortunately something like this might be fixed by the changes in #10550 by using std::chrono::steady_clock instead.

[yhabteab]

Edit: It might be because of a clock anomaly where the runner might have adjusted time back a second which would affect HttpServerConnection, but not the test case. Fortunately something like this might be fixed by the changes in #10550 by using std::chrono::steady_clock instead.

I think, I have seen the error with #10550 before I changed it to use std::chrono::steady_clock as well and I just retried it without thinking that much about it, so you might be right about this.

[julianbrost]

I think, I have seen the error with #10550 before I changed it to use std::chrono::steady_clock as well and I just retried it without thinking that much about it, so you might be right about this.

Was this also on Windows?

Edit: It might be because of a clock anomaly where the runner might have adjusted time back a second which would affect HttpServerConnection, but not the test case. Fortunately something like this might be fixed by the changes in #10550 by using std::chrono::steady_clock instead.

Sounds plausible, but also like it would take quite some digging to confirm. Anyway, this PR only touches HTTP handlers, which aren't even accessed by that test case (it only creates a idle connection), so really nothing to do here in this PR. Either #10550 just fixes it and we're happy, or if it happens again, we might want to take a closer look.

[yhabteab]

Rebased and addressed all the requested changes:

• 68deaaf refactors the existing ValueGenerator class and makes it templated, so that it can be used with any std container.
• 3f54253 removes the ActionsHandler::AuthenticatedApiUser and passes the api user as param to the actions instead.

[yhabteab]

I think, I have seen the error with #10550 before I changed it to use std::chrono::steady_clock as well and I just retried it without thinking that much about it, so you might be right about this.

Was this also on Windows?

Edit: It might be because of a clock anomaly where the runner might have adjusted time back a second which would affect HttpServerConnection, but not the test case. Fortunately something like this might be fixed by the changes in #10550 by using std::chrono::steady_clock instead.

Sounds plausible, but also like it would take quite some digging to confirm. Anyway, this PR only touches HTTP handlers, which aren't even accessed by that test case (it only creates a idle connection), so really nothing to do here in this PR. Either #10550 just fixes it and we're happy, or if it happens again, we might want to take a closer look.

Apparently, Windows has some serious issues. Now, Win64 is failing due to the newly changed timer/invoke intervals. It didn't fail before in the PR that changed the intervals, but now it does. I don't understand this ** OS at all.

[jschmidt-icinga]

I think the new ValueGenerator is a good improvement on the interface side without complicating the implementation by much. But I do have one small request about the test cases (see below).

[jschmidt-icinga]

Unless you've changed anything besides resolving the merge-conflicts, this is still fine from my side.

[yhabteab]

Unless you've changed anything besides resolving the merge-conflicts, this is still fine from my side.

Yes, I didn't change anything apart from resolving the conflicts, which involved removing some codes regarding the ticket salt and the generator being able to skip elements.

[julianbrost]

Oh, and it has merge conflicts now that need to be resolved as well 😅

[julianbrost]

See #10554 (review). With that, this PR probably shouldn't show as approved in lists :)

[yhabteab]

The end result of our discussion from today about this is as follows:

We won't break any existing handler by changing their HTTP status code to 202. Instead, the /v1/actions and /v1/objects/modify handlers will stream their response after fully processing the request, and thus will not have any behavioral change as opposed to the current implementation. The /v1/objects/query and other (read-only) handlers will also not change their HTTP status code either, but will still stream the response on the fly as it is being generated, which doesn't change their behavior in any way.

---

End of the discussion. If I missed anything please extend this message with the missing information.

[julianbrost]

Looks good to me now, thank you! I know, it's not really what we all had hoped for in the beginning, but sometimes things aren't as simple as one thought, for everything else not done in this PR, there's now #10717.

I'm not merging this immediately as there were quite some changes recently. So if anyone wants to take another look, now is your chance.

[julianbrost]

fixes #9535

I've now removed this from the PR description as this PR no longer contradicts that issue.

[jschmidt-icinga]

Also gave it another quick look and still looks fine to me.