=C2=A0
Ed,

Thank you so much for quick res= ponse.=C2=A0

>=C2=A0 =C2=A0 1) Create StreamSocketRule and StreamSocket classes as li= ke as websocket
>=C2=A0 =C2=A0 2) Add one more RuleParameterTraits for StreamSocketTwo w= ays to invoke this new class -
>=C2=A0 =C2=A0 =C2=A01) Create static bmcweb route for each logservice w= ith dump uri=C2=A0 with specific rule trait for streamsocket
>=C2=A0 =C2=A0 =C2=A0 =C2=A0eg:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0BMCWEB_ROUTE(app, "/system/LogSe= rvices/Dump/attachment/<str>",id)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.privileges({"ConfigureComponent= s", "ConfigureManager"})
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.streamsocket()

I'd much rather call it "dyamicResponse" and have it take 3 c= allables,
one for onRequest, that accepts a Request and a Response object, like
a normal handler, one for onData, that accepts a buffer of the body
data, and one for onClose, that informs the calling code the socket
was closed.

=C2=A0
=C2=A0
Exactly, we thought to imp= lement 4 callables in new rule class similar to=C2=A0 websocket
=C2=A0 = =C2=A0OnOpen(),OnMessage(),OnClose(),OnError() .=C2=A0 We can use names as = suggested.=C2=A0

Dump dump offload happens using NBD protocol, it= 9;s like NBD over http.
In this=C2=A0 streaming use-case, data will be b= idirectional as there will be nbd acknowledgement
for each nbd packet t= ransferred to client. so thought to use=C2=A0"StreamSocket" name.= =C2=A0

>=C2=A0 =C2=A0 =C2=A02) Create another redfish node class(as the existin= g node class is for dynamic route)=C2=A0

which would call app.route with the streamSocke= t trait.Do you have any preference here?handleUpgrade() of router class get= s invoked in the following conditions -

The Redfish Node class is specifically for request->response type
operations, and in the long run, I'd really like to see it just go
away.=C2=A0 Today, it doesn't do anything more than the underlying rout= er
already does, and is missing a number of things, like typesafe
parameter parsing, as well as copy-less parameters.=C2=A0 If it were me, I<= br> would simply model your dynamic responses around BMCWEB_ROUTE=C2=A0
=C2=A0
>=C2=A0 =C2=A0 =C2=A0a)request contains http header(upgrade:websocket) >=C2=A0 =C2=A0 =C2=A0b)request contains http header (accept: text/event-= stream) ---> yet to be upstreamedIn our use case for dump stream, we do = not need to take this decision by looking at the request header as the URL = is already registered with their associated rule(StreamSocketRule) class.Wh= en we search in the trie for a specific URL, we will get the associated rul= e class object, which in our case would be StreamSocketRule and the handle = function of this class should make sure that socket is not closed.
> Is my understanding correct understanding or do you have more suggesti= on on this?There is an active discussion going on in the following commit f= or the same.
> https://gerrit.openbmc-project.xyz/c= /openbmc/bmcweb/+/36038

I think you've described what bmcweb does today.=C2=A0 In this case, we=
might want to just promote an "isUpgrade" method from the app, th= at we
can use to determine if a given route is dynamic or not, and call into
the correct "handleStream" operator.=C2=A0 Keep in mind that at s= ome point
you need to transfer control back to httpConnection to handle
keepalive properly.
=C2=A0
Thinking about keepalive more gave me another thought: This isn't
really an "upgrade" in the normal sense.=C2=A0 What if we just ad= ded a
"write" method to the Response object that immediately wrote the<= br> payload to the dynamic body and sent it?=C2=A0 write() could return an
error code in the cases where the client has disconnected, and we
never have to transfer ownership or knowledge of the socket, and the
keepalive and end() mechanisms would continue to work like normal.
HttpConnection would have to get some knowledge about whether this
request was dynamic or not, but that seems pretty doable, and could
just be based on the first call to write.=C2=A0 Also, this keeps all the keepalive code the same, which I think is good.

That's exactly why we thought to use handle() met= hod, but there is a gap how to transfer ownership
of so= cket from connection class to rule class.

In t= he existing implementation where we transferred ownership of socket from co= nnection class to rule class
based on=C2=A0 http header field &qu= ot;upgrade::websocket"

=C2=A0As I explained above, we need bi-d= irectional communication, where bmc sends certain=C2=A0payload and nbd on c= lient-side
acknowledges received payload.

So we need webso= cket way of implementation, where we need to keep reading and writing const= antly on the same socket.

What I am unable to connect is how to tran= sfer=C2=A0ownership of socket connection to new rule class, as in this case= =C2=A0
we can't take the decision based on=C2=A0 request head= er/content. can you provide your suggestion=C2=A0 please?

What do yo= u mean by dynamic request and dynamic response?
As per my understanding,= dynamic response is based on http request content, one of the header field= =C2=A0"accept"
where client specifies data format and d= epending on this format, response=C2=A0gets generated. Is it=C2=A0a dynamic= response?
if it's true, how is it applicable here?

Response object writing payload to dynamic body may not work in th= is case.
Response object does not=C2=A0 hold=C2=A0 socket, only connecti= on class which is having socket, except handleUpgrade case
where we tra= nsfer=C2=A0socket ownership to connection to rule class which creates anoth= er connection

Thanks
-Raviteja
=C2=A0

>> On Fri, 28 Aug, 2020, 12:42 am Ed Tanous, <ed@tanous.net> wrote:
>>>
>>> On Sun, Aug 2, 2020 at 8:53 AM Ed Tanous <ed@tanous.net> wrote:
>>> >
>>> > I'm looking at a couple recent changes to bmcweb, and= I'm finding a
>>> > significant architecture problem has been injected.=C2=A0= Namely, it's
>>> > these innocuous looking 4 lines here, which injects the s= ocket adaptor
>>> > into the request object for use later.
>>> > https://github.com/openbmc/bmcweb/blob/30c58d581606b44= 84757e6ee9133c248de1514a6/http/http_request.h#L18
>>> >
>>> > The problem with this approach has a few roots:
>>> > 1. The Request class is meant to model a single request, = single
>>> > response model.=C2=A0 Adding the stream semantics breaks = this in pretty
>>> > significant ways, and forces a hard dependency between th= e streaming
>>> > adapter and the Request, which was not the intent.=C2=A0 = We have
>>> > abstractions for "streaming" requests, but that= was seemingly not
>>> > used.
>>> >
>>> > 2. In the code that existed before this, Adaptor was a te= mplate on
>>> > purpose.=C2=A0 It is designed to implement the std::netwo= rking
>>> > AsyncReadStream and AsyncWriteStream concepts.=C2=A0 This= is designed to
>>> > allow injection of Unit Tests at some point, as I've = talked about
>>> > before.=C2=A0 Hardcoding it in request to 2 forced stream= types, based on
>>> > the compiler flag is incorrect per asio standards, and re= moves the
>>> > ability to inject arbitrary adapters, like test adaptors.= =C2=A0 Also, the
>>> > flag used is incorrect, as it's possible to inject a = non-ssl socket
>>> > even if SSL is enabled.
>>> >
>>> > 3. There is already a precedent and pattern for streaming= interfaces
>>> > in bmcweb that we adopted from Crow.=C2=A0 If you look at= the Websocket
>>> > request response type, it implements a way to request a r= oute that
>>> > streams dynamically.=C2=A0 Frustratingly, part of what th= is was used for
>>> > was SSE, which I had already written a patch for that did= n't have any
>>> > of the above issues, and only hadn't merged it becaus= e we didn't have
>>> > any SSE routes yet, and didn't want to check in dead = code.
>>> > https://gerrit.openbmc-= project.xyz/c/openbmc/bmcweb/+/13948
>>> >
>>> > 4. It opens the possibility for lifetime and possible (at= the very
>>> > least harder to audit) security issues, as now the "= http server"
>>> > component is no longer the only thing that can own socket= s.
>>> > Previously, the server owned the sockets until handed off= , then there
>>> > was no shared ownership between the websocket class, and = the
>>> > Connection class.=C2=A0 The Connection class could be com= pletely destroyed
>>> > (and memory freed) while the websocket was still connecte= d and
>>> > running.
>>> >
>>> > Moving to another track, you may ask, how did I come acro= ss this and
>>> > why does it matter?=C2=A0 I'm trying to add 2 new fea= tures to bmcweb.=C2=A0 The
>>> > first allows opening multiple sockets, and dynamically de= tecting TLS
>>> > streams on them.=C2=A0 This allows bmcweb to handle both = HTTPS redirects in
>>> > band, and handle the case where users type in something e= rroneous,
>>> > like "http://mybmc:443" and connect to an SSL socket= with a non-ssl
>>> > protocol.=C2=A0 In those cases, we can simply do the righ= t thing.=C2=A0 It also
>>> > allows bmcweb to host on multiple ports, which might be i= nteresting
>>> > for aggregator types.=C2=A0 More importantly, it cleans u= p some of the
>>> > Adaptor abstraction to make way for unit testing, and bei= ng able to
>>> > inject a "test" socket, that we can control the= semantics of.=C2=A0 I'm
>>> > hoping eventually to be able to mock dbus, and mock the T= CP socket,
>>> > and run a full Redfish validator run in a unit test.=C2= =A0 I think that
>>> > would save a lot of time overall for both committers and = consumers.
>>> >
>>> > The first of these patches is posted here, and simply com= ments out the
>>> > above problems for now.
>>> > https://gerrit.openbmc-= project.xyz/c/openbmc/bmcweb/+/35265
>>> >
>>> > If I look through the commit logs, it looks like Ravi and= Appu built
>>> > the two small subsystems that rely on the above abstracti= on, one for
>>> > SSE, and one for some NBD streamer.
>>> > What do you two think about the above?=C2=A0 Was it somet= hing you
>>> > considered when you wrote your patches?=C2=A0 Would you c= onsider fixing
>>> > them?
>>> >
>>> >=C2=A0 =C2=A0My recommendation would be to move both of th= ose two over to
>>> > something similar to the websocket abstraction we have, w= ith, on
>>> > connect, on data, and on close handlers.=C2=A0 This means= that handlers no
>>> > longer take a hard dependency on the transport, which wil= l help for
>>> > both unit testing, and if we ever want to support redfish= device
>>> > enablement (which relies on an i2c based transport). The = SSE one can
>>> > probably be used more or less as-is from my old patch.=C2= =A0 The NBD one
>>> > might need a "Dynamic body" type, which beast a= lready has an
>>> > abstraction for that seems to have been discounted.
>>> >
>>> > What do you guys think?
>>> >
>>> > -Ed
>>>
>>>
>>> It's been 3 weeks and I haven't gotten any replies to = this pretty
>>> major architecture break.=C2=A0 It also looks like it can also= cause a
>>> memory leak in HttpConnection here (found by code inspection h= ere).
>>> https://github.com/openbmc/bmcweb/blob/ebd459060ea4f427= 61402dd54acd0962c36136c2/http/http_connection.h#L351
>>>
>>> I've pushed a revert to remove the features in question.= =C2=A0 I would love
>>> some comments from the developers that caused these breakages,= so I
>>> can make sure I'm doing the right thing here, and I'm = not completely
>>> off base (or that you intend to fix them, and this patch is >>> unnecessary).
>>> https://gerrit.openbmc-proje= ct.xyz/c/openbmc/bmcweb/+/36038
>>>
>>> Thanks,
>>>
>>> -Ed

--00000000000009f8d105ae67c762--