search

cloudfoundry / gorouter

CF Router
Apache License 2.0
441 stars 226 forks source link

Experiment: `http.ResponseController.EnableFullDuplex()` #380

Closed peanball closed 9 months ago

peanball commented 11 months ago

Experiments with http.ResponseController.EnableFullDuplex().

mariash commented 9 months ago

EnableFullDuplex is currently failing on the ResponseController since ResponseWriter that is received in ServeHTTP is Negroni.ResponseWriter which does not provide EnableFullDuplex. There is an issue opened about Negroni being outdated.

peanball commented 9 months ago

Yes, I've opened that issue after attempting with this branch. The PR was to share progress with @geofffranks and can be closed now. I will do that latest next week when I'm back at work.

peanball commented 9 months ago

Closing this PR. It was an experiment with EnableFullDuplex that didn't work and resulted in https://github.com/cloudfoundry/routing-release/issues/373.

mariash commented 9 months ago

@peanball I am looking into this right now. I think I would make PR to negroni to add this.

peanball commented 9 months ago

Negroni looks like a dead project through.

Before making a PR there we might consider a locally patched version, try if it gives us the gains that @geofffranks was hoping for and only then consider resurrecting this project.

That said, there might be other features we might like from newer Go versions now or later that would have to be added by us to Negroni instead of a framework's maintainers.

mariash commented 9 months ago

@peanball so I looked into this more and negroni provides an Unwrap method to the ResponseWriter - https://github.com/urfave/negroni/commit/59d32aab419da1606c459c848c7586ef52b07a4b which will satisfy the latest ResponseController and eventually EnableFullDuplex will be called on the underlying http.ResponseWriter.

Negroni actually has v2 and v3 versions, that are not visible on their releases page, but can be imported as github.com/urfave/negroni/v3. The problem is that Unwrap was not released on v3 yet. And looking at Issue the last comment from December 2023 stated that author is temporarily unavailable. So I would give it couple more months to see if he will be back and cut a new release. For now I pinned the negroni to latest v3.

Here is the proposed change for the Gorouter: https://github.com/cloudfoundry/gorouter/pull/395 Related change in routing-release: https://github.com/cloudfoundry/routing-release/pull/389

peanball commented 8 months ago

Negroni actually has v2 and v3 versions, that are not visible on their releases page, but can be imported as github.com/urfave/negroni/v3. The problem is that Unwrap was not released on v3 yet.

that's a great find, @mariash!

Admittedly I just checked the GH releases page, not the tags...