Closed kerams closed 2 years ago
Awesome work as always @kerams ❤️
Before I publish this, I was actually checking out the implementation in Giraffe and it seems that Giraffe now registers a default RMS. Is it an idea for Remoting to actually try grab that registered RMS and use internally by default? i.e. use what's available:
HttpContext
, Also, what do you think about Remoting.withRecyclableMemoryStreamManager
being of type HttpContext -> RecyclableMemoryStreamManager
?
Finally, maybe a small rename
withRecyclableMemoryStreamManager
->withStreamManager
?
There's one problem. buildFromImplementation
creates a proxy with the stream manager before there are any requests and HTTP contexts. In other words, at that point it's not (easily?) possible to look up services registered in the DI container.
Also, what do you think about Remoting.withRecyclableMemoryStreamManager being of type HttpContext -> RecyclableMemoryStreamManager?
I don't quite know what you want to achieve with that :).
withRecyclableMemoryStreamManager -> withStreamManager?
Sure.
Also, what do you think about Remoting.withRecyclableMemoryStreamManager being of type HttpContext -> RecyclableMemoryStreamManager?
I don't quite know what you want to achieve with that :)
I want to make it easy to require the already injected RMS from Giraffe:
|> Remoting.withStreamManager (fun (context: HttpContext) ->
let rms = context.GetService<RecyclableMemoryStreamManager>()
Some rms
)
Without having to define one on your own. Does this make sense?
Should a default implementation already do that so that users of Giraffe & Remoting get a shared RMS without doing extra work?
That would be unusable in Suave as it does not have any built-in DI as far as I know. We could have both your function and mine to configure the RMSM, but it would be a bit silly to complicate things so much for something almost no one will really bother to customize.
I think what we have right now is the best solution since it's webserver/DI-agnostic. No injection through any containers, just direct plugging in via RemotingOptions
.
I think what we have right now is the best solution since it's webserver/DI-agnostic
Then I'll leave it as is, I trust your judgement 💯 published a new batch of packages that includes the feature
Just some more perf optimizations by reusing temporary output streams (we avoid reallocating an output buffer for every response) thanks to Microsoft.IO.RecyclableMemoryStream. Giraffe already uses this internally, so for the majority of users this won't be a new dependency.
I've also added
Remoting.withRecyclableMemoryStreamManager
This aligns with https://github.com/giraffe-fsharp/Giraffe/pull/494, so that you could use recyclable memory streams on your own, in Giraffe and in Remoting, and all 3 would share the same memory stream pool (which has a couple of settings you can tune) if you wish.