Fix bug where reusing a pklbinary#Renderer could result in incorrect output

[HT154]

This code triggers the error condition (where the b64 scheme is implemented by a custom/external resource reader):

import "pkl:pklbinary"
abstract class Base {
  fixed kind: String
  input: String
  local encodedRequest = new pklbinary.Renderer {}.renderValue(this).base64
  hidden fixed requestUri: String = "b64:\(encodedRequest)"
  hidden fixed result: Resource = read(requestUri) as Resource
}
class Enc extends Base {
  fixed kind: "enc"
}
class Dec extends Base {
  fixed kind: "dec"
}
local enc = new Enc { input = "hello world" }
local dec = new Dec { input = enc.result.text }
valid = if (enc.input == dec.result.text) true else throw("pklbinary.Renderer reuse bug")

The data dependency here is dec.result.text -> read of dec.requestUri -> dec.encodedRequest -> dec.input -> enc.result.text -> read of enc.requestUri -> enc.input

When reusing the MessagePacker, the read that is executed second (dec's) returns the result of the first read.
I wasn't able to determine exactly why this happens, but it's obvious that the messagePacker.clear() call isn't having the intended effect.
Attempting to replace it with messagePacker.reset(<new buffer>) did not help either.

This is probably a negligible memory/perf hit in favor of correctness.

[odenix]

Because this is stdlib code, there is only a single instance of renderDocument/renderValue, shared across all Evaluators. Reusing the same MessageBufferPacker across concurrent eval() calls isn't thread-safe.

In a server application, I’d hesitate to create a new packer for every call, but I’m not sure how much it matters here. If eval() calls can trigger nested eval() calls (and I suspect they can), there may be no better option. Otherwise:

pkl-server uses ThreadLocal<MessageBufferPacker>, but that risks retaining too many (or overly large) instances over time, especially since pkl-core is a library and doesn't control how threads are managed. Perhaps an alternative is to keep one packer per VmContext, so it can be garbage-collected together with the corresponding Evaluator.

[bioball]

Perhaps an alternative is to keep one packer per VmContext, so it can be garbage-collected together with the corresponding Evaluator.

I think the issue here is that renderValue can be called within another renderValue, right? If so, then sharing a packer per VmContext wouldn't fix this problem either.

[bioball]

LGTM

[odenix]

Yes, that’s what I suspected. In that case, this looks fine to me. VmContext just solves multi-threading issues. Could perhaps still use a thread-safe pool (as I do in mxpack), but that can wait.