Move synth reason between workspaces from vcl_pipe

[Dridi]

In vcl_pipe req is read-only and we operate bereq as a busy object, as a
result we use the busy object's workspace to back VCL expressions. When
we use the synth transition we may build a reason field on the wrong
workspace since vcl_synth carries on in a client context.

It is possible to notice that the reason string belongs to the backend
workspace and make a copy inside the client workspace before the former
is released.

Fixes #3329

[nigoroll]

I can see how this fixes #3329, but I wonder if we should really point-fix it. What about, for example, if we used PRIV_TASK scope in vcl_pipe {} and expected that to be available in vcl_synth {}?

That is to say, wouldn't it make sense to go all the way and move vcl_pipe {} entirely to the backend side?

[Dridi]

We had an informal review in my team and my conclusion is that we should probably keep the backend workspace around (and as such the busyobj).

If we jump to vcl_synth that means we also need to release it at VCL_TaskLeave() time, either when the transaction ends or when we return (restart). I was planning to mention it during bugwash when this or #3329 would be discussed.

edit: and add a bo->is_pipe flag not exposed to VCL

[nigoroll]

@Dridi the point I wanted to make with my last comment is that vcl_pipe {} seems to be more fundamentally broken:
Our ctx.ws is that of the busyobj, yet we are in the client task. So it is not just the synth() reason which gets lost, but we would also lose any PRIV_TASK when restarting or transitioning to vcl_synth (actually, produce an invalid pointer).
Thus, I wonder if it made more sense to move vcl_pipe entirely to the backend side.

[Dridi]

Thus, I wonder if it made more sense to move vcl_pipe entirely to the backend side.

I don't think it's a good idea to spawn a thread for a transaction that can virtually last forever, holding onto a client thread while at it.

If anything, I could see us going the other way around and make the pipe transaction strictly a client-side thing where either the busyobj and all the privs are managed inside workspace_client or we come up with a dedicated struct that is not busyobj for pipe mode but that might complicate the director/backend integration.

So it is not just the synth() reason which gets lost, but we would also lose any PRIV_TASK when restarting or transitioning to vcl_synth (actually, produce an invalid pointer).

I would rather think that the PRIV_TASK problem is that we lose client privs in vcl_pipe despite being in the client state machine.

[nigoroll]

Re @Dridi I did not mean to suggest to run pipe in another thread.
But you are right, it probably makes more sense to do the opposite and move all of vcl_pipe {} to the client side as that makes the PRIV_TASK more useful to begin with.

[Dridi]

We've discussed it offline but instead of trying to move things between workspaces I would either consider vcl_pipe a client-side step and put everything in workspace_client, or keep workspace_backend around until the task ends (vcl_synth delivers or restarts). I'm leaning towards the latter.

Keeping open for the sake of discussion.

[nigoroll]

I came across this again in the context of 3dc2bae and again stumbled over the duality of vcl_pipe {} between the client and backend side.
For the shard director, I now decided to treat it like the backend side (because the director resolution happens there), but I see Dridi having a point that the client restart (and optionally rollback) would see it on the client side.
I guess the most compatible way would actually be Dridis suggestion to just keep the backend workspace around until a pipe client request has finished, and also we would need to special-case rollback from vcl_pipe (to also rollback the backend side).
But still, conceptually, moving pipe mode to the backend side (yet running it in the existing client thread) would seem to make more sense to me.
Related to that, I would like to reiterate a bit: Why do we need the pipe->synth transition in the first place? What is the use case for deciding on synth after recv? IINM, synth would be the only case we would still needed the client workspace, fail should not need any and if we moved it to the backend side, conceptually, restart would be replaced by retry.

[Dridi]

I came across this again in the context of [...]

Yes, and you referenced this pull request, and that's when I realized I never added the conclusions from our offline discussion.

Why do we need the pipe->synth transition in the first place?

I don't see a good answer to this :-/

[Dridi]

This is definitely a path we should no pursue, but we still have to reach consensus on how to fix #3329. #3408 looks like a better forum for that discussion.