Uh oh!
There was an error while loading. Please reload this page.
Documenting and testing SetupResumeAcceptor - #595
Conversation
| case FrameType::RESERVED: | ||
| case FrameType::PAYLOAD: | ||
| case FrameType::RESUME_OK: | ||
| case FrameType::EXT: |
There was a problem hiding this comment.
I would suggest to keep these cases in, as its required by the compiler warning in -Wextra mode.
I found 2 bugs in RSocket in the past in this.
There was a problem hiding this comment.
They shouldn't be necessary, as there's a default label that captures all of them. I don't remember a warning included in -Wextra that would trigger that, are you thinking of -Weverything instead?
Also note that there are no warnings in the builds despite us using -Wall -Wextra -Werror for code in rsocket/.
There was a problem hiding this comment.
Given that we really only care about SETUP and RESUME here I think default makes sense.
| const folly::IOBuf& firstFrame) { | ||
| if (defaultFrameSerializer_) { | ||
| return defaultFrameSerializer_; | ||
| std::shared_ptr<FrameSerializer> SetupResumeAcceptor::frameSerializer( |
There was a problem hiding this comment.
I would suggest to add the word "detect" in the name of the method.
frameSerializer() doesn't suggest its an "expensive method" doing some heuristic to return FrameSerializer.
There was a problem hiding this comment.
Fair enough, I can see how frameSerializer() might be construed as being a simple getter. Can I convince you on createSerializer()? I like keeping method names short, and the create part suggests that there's some expensive work being done. Plus the method has a comment saying that it'll do the autodetect logic.
lexs
commented
Jul 19, 2017
Nice! There used to be tests for this class but they were deleted in #445. Last version was https://github.com/rsocket/rsocket-cpp/blob/f154f0c1fde388c660c0f7ee3724bb954ad14c2b/test/ServerConnectionAcceptorTest.cpp. Could you add the ones that make sense back? They tested for some very real bugs that we saw in prod. |
alexmalyshev
commented
Jul 19, 2017
Sure, I can take a look through them and add back what I can. |
lexs
left a comment
There was a problem hiding this comment.
Looks good, we can defer the case discussion if it's important but I'd like the tests here to get in.
alexmalyshev
commented
Jul 24, 2017
Sorry, I'm intentionally delaying this as @somasun convinced me the code in https://github.com/rsocket/rsocket-cpp/blob/master/rsocket/internal/SetupResumeAcceptor.cpp#L149 was necessary for correctness. I want to try exposing that crash in the tests first before landing. |
| void SetupResumeAcceptor::closeAndRemoveConnection( | ||
| const yarpl::Reference<FrameTransport>& transport, | ||
| void SetupResumeAcceptor::close( |
There was a problem hiding this comment.
from SetupResumeAcceptor::close name I would guess its closing the SetupResumeAcceptor, not just the connection. what is wrong with closeAndRemoveConnection?
There was a problem hiding this comment.
Too long, often clips past 80 columns. I think reading close(transport, exn) is pretty unambiguous about closing transport.
| public: | ||
| using OnSetup = | ||
| std::function<void(yarpl::Reference<FrameTransport>, SetupParameters)>; | ||
| folly::Function<void(yarpl::Reference<FrameTransport>, SetupParameters)>; |
* Tweaked OnSetup and OnResume functions to be folly::Function to keep us honest about moving them instead of copying. * Forced use of folly::EventBase::runInEventBaseThread() on all calls to close() in SetupResumeAcceptor::processFrame(). * Fixed any places where we were constructing a folly::exception_wrapper without a std::exception_ptr. * Added MockDuplexConnection ctor that runs lambdas on input and output subscribers.
Documenting and testing SetupResumeAcceptor
Tweaked OnSetup and OnResume functions to be folly::Function to keep us
honest about moving them instead of copying.
Forced use of folly::EventBase::runInEventBaseThread() on all calls to close() in
SetupResumeAcceptor::processFrame().
Fixed any places where we were constructing a folly::exception_wrapper without
a std::exception_ptr.
Added MockDuplexConnection ctor that runs lambdas on input and output
subscribers.