Fuerte simplify #12270
Fuerte simplify #12270
Conversation
| // TODO: should we have a better timeout mechanism? | ||
| // this->setTimeout(GeneralCommTask<T>::DefaultTimeout); |
There was a problem hiding this comment.
| // TODO: should we have a better timeout mechanism? | |
| // this->setTimeout(GeneralCommTask<T>::DefaultTimeout); |
| if (_protocol == "http" || _protocol == "h1") { | ||
| config.protocol = fuerte::ProtocolType::Http; | ||
| } else if (_protocol == "http2" || _protocol == "h2") { |
There was a problem hiding this comment.
| if (_protocol == "http" || _protocol == "h1") { | |
| config.protocol = fuerte::ProtocolType::Http; | |
| } else if (_protocol == "http2" || _protocol == "h2") { | |
| if (_protocol == "http2" || _protocol == "h2") { |
This is a second place where _active can be set to FALSE. We better do the model exactly as in the implementation.
There was a problem hiding this comment.
Overall: Well done! This is a good simplifaction.
I have committed changes to the model, since it did not fit to the implementation. However, this already was the case before your changes. The model now does shutdownConnection as the implementation does, rather than setting the state to Closed and then calling asyncWriteNextRequest. This now allows to add a few more assertions, which seems sensible. I have commited my suggested changes to the branch feature/fuerte-simplify-suggestion. This also contains some cleanups in comments, some dead code and bad comment removal. Just have a look.
3rdParty/fuerte/src/H1Connection.h
Outdated
| std::chrono::steady_clock::time_point _writeStart; | ||
| // This is the time when the latest write operation started. | ||
| // We use this to compute the timeout of the corresponding read if the | ||
| // write was done. | ||
|
|
||
| std::atomic<int> _leased; |
There was a problem hiding this comment.
I think this was forgotten to remove. It is no longer used.
There was a problem hiding this comment.
Done in branch feature/fuerte-simplify-suggestion.
3rdParty/fuerte/FuerteH1TLS.tla
Outdated
| @@ -22,10 +22,10 @@ variables \* VERIFIED | |||
|
|
|||
| procedure startConnection() { \* VERIFIED | |||
| (* This is called whenever the connection broke and we want to immediately | |||
There was a problem hiding this comment.
This comment is out of date and should be corrected.
| // start connecting only if state is disconnected | ||
| Connection::State exp = Connection::State::Created; | ||
| if (_state.compare_exchange_strong(exp, Connection::State::Connecting)) { | ||
| FUERTE_LOG_DEBUG << "startConnection: this=" << this << "\n"; |
There was a problem hiding this comment.
The model suggests that we can add an assertion on _active here. We should at least try this, if it breaks the tests, then we have a discrepancy between the model and the implementation.
There was a problem hiding this comment.
Done in branch feature/fuerte-simplify-suggestion.
3rdParty/fuerte/src/H1Connection.cpp
Outdated
| @@ -485,46 +341,19 @@ void H1Connection<ST>::asyncWriteCallback(asio_ns::error_code const& ec, | |||
| FUERTE_ASSERT(this->_state != Connection::State::Connecting); | |||
There was a problem hiding this comment.
The comment above is outdated: The state is now called Closed and it can happen for TCP or for TLS connections. Please correct.
There was a problem hiding this comment.
Changed in branch feature/fuerte-simplify-suggestion
3rdParty/fuerte/src/H1Connection.cpp
Outdated
| @@ -485,46 +341,19 @@ void H1Connection<ST>::asyncWriteCallback(asio_ns::error_code const& ec, | |||
| FUERTE_ASSERT(this->_state != Connection::State::Connecting); | |||
There was a problem hiding this comment.
The model suggests that state Created cannot happen here. Let's adjust the code to this.
There was a problem hiding this comment.
Done in branch feature/fuerte-simplify-suggestion.
3rdParty/fuerte/src/H1Connection.cpp
Outdated
| << static_cast<int>(state) | ||
| << " instead of the expected 'Connected'\n"; | ||
| } | ||
| state == Connection::State::Closed); |
There was a problem hiding this comment.
This can actually now be changed to only Connected, since we no longer call asyncWriteNextRequest when the connection is closed. See model.
There was a problem hiding this comment.
Done in branch feature/fuerte-simplify-suggestion.
3rdParty/fuerte/src/H2Connection.cpp
Outdated
| void H2Connection<T>::terminateActivity() { | ||
| // TODO: fill in the necessary stuff to fix bugs in this class | ||
| } | ||
| //template <SocketType T> |
There was a problem hiding this comment.
Let's remove that dead code. Done in branch feature/fuerte-simplify-suggestion.
| // TODO: should we have a better timeout mechanism? | ||
| // this->setTimeout(GeneralCommTask<T>::DefaultTimeout); |
| RequestOptions const& options, Headers headers) { | ||
| RequestOptions const& options, Headers headers, | ||
| std::chrono::duration<double> timeout) { | ||
| TRI_ASSERT(path.find("/_db/") == std::string::npos); |
There was a problem hiding this comment.
This looks dangerous to me. Why would this never show up in an path of a URL? Granted, it is only an assertion and so it would only impact us in the tests, but still. I would like to understand the rationale behind this.
There was a problem hiding this comment.
OK, I think I understand, one is supposed to use the database option. Maybe only assert that the path does not begin with /_db/?
| if (_protocol == "http" || _protocol == "h1") { | ||
| config.protocol = fuerte::ProtocolType::Http; | ||
| } else if (_protocol == "http2" || _protocol == "h2") { |
Fuerte connections no longer perform reconnects internally which should make it easier to reason about it.
http://jenkins01.arangodb.biz:8080/job/arangodb-matrix-pr/11191/