pd: Routes added to tonic server cause regression in http trailers
#3697
Comments
|
hyperium/tonic#455 notes two unfinished issues:
(emphasis mine) i suspect that the change to use a i'll take a look tomorrow, but i plan to keep a tight timebox on attempting to land that PR again, as written. it was addressing a it would be nice to test pd against a grpc-web client, if we do not already. that feels outside the scope of this issue. thanks for opening this and for reverting that commit, @conorsch! (and thank you to everybody else that helped track this regression 🐛 🔍) |
|
Another possibility is that we could file an issue upstream, and then keep the commit around to pull up again if it gets resolved -- the new code is certainly nicer but i'm not sure it's critical to land it at the moment. |
cratelyn
added
A-node
cratelyn
changed the title
Routes added to tonic server cause regression in http trailers
|
note: i filed #3704 to track the addition of integration tests exercising grpc-web compatibility, to keep that conversation separate from this specific regression. |
see #3913, #3973 and #3588. this is a second attempt, following up on #3980. #### 🔭 background NB: the difference between this and #3679 is that the latter (_which ran afoul of a regression_) would have `penumbra-app` create a `Routes`, that we would [add](https://github.com/penumbra-zone/penumbra/pull/3679/files#diff-fbc4204ceb976c8cb30ed06168e2476700bae21bfd803e26281b2d026194d430R204) to the builder (_which stays in `pd`_). here, i'm not trying to make that cut between `Router` and `Routes`, and am attempting to hoist the whole thing out of `pd`, without making changes to how we interact with `tonic`. my aim is for us to be able to move this, without running into that bug (#3697) again. NB: after running into problems in #3980, i found a way to easily reproduce the issue locally. my belief was that something related to our dependencies' cargo features was at play. rather than isolate the issue, it was easier to rewrite this (_it's just code motion, after all_) while running some of the network integration tests in a loop. unlike #3980, this moves the rpc server into `penumbra-app`, per #3980 (comment) #### 👁️ overview we would like to use the rust view server in mock consensus tests. in order to run the `penumbra_view::ViewServer` however, we need to spin up the corresponding grpc endpoint for it to connect to. this branch performs a bit of code motion, moving the `grpc_server` out of `pd` and into `penumbra-app`. there will likely be other functional changes to the code in question before we can use it in those tests, but this PR is interested in moving that code into a place where our tests can rely upon it.
We observed a breaking change in the local dev env for web extension development this morning, caused by merge of #3679. Although that PR passed smokes and even manual testing via pcli, it broke the web extension, due to the refactor no longer supporting HTTP trailers.
For now, we'll revert that commit, to unbreak web development immediately. We should also investigate how to improve integration testing between the penumbra monorepo and the latest tip of extension code.
Misc
@hdevalence isolated the failing commit via git bisect.
The text was updated successfully, but these errors were encountered: