Decoupling TCP Connection from WriterRef and ReaderRef #119
Conversation
| debug!("writer loop is shutting down"); | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
I know in this codebase we put the tests on a separate file but due to the module visibility restrictions I put it here just for showing the use case. Can move the test to a separate file and or get rid of it depending on the outcome of the discussion
There was a problem hiding this comment.
We very much put unit tests in the same file. I only broke out the message store tests because spinning up a mongodb container in unit tests felt wrong.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #119 +/- ##
==========================================
+ Coverage 54.05% 54.83% +0.78%
==========================================
Files 59 60 +1
Lines 2653 2659 +6
==========================================
+ Hits 1434 1458 +24
+ Misses 1219 1201 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nnel read and writer
DGPardo
force-pushed
the
decoupling-tcp-connection-from-io
branch
from
9f856d8 to
a3c383f
Compare
davidsteiner
left a comment
davidsteiner
left a comment
There was a problem hiding this comment.
I think we should take a step back and start breaking out the modules that are related to a real FIX connection over TCP, and the generic bits that don't care about the actual connection.
| break; | ||
| } | ||
| #[async_trait] | ||
| impl WriterModel for WriterRef { |
There was a problem hiding this comment.
Why do we need to implement a trait for WriterRef? Other than the new function, it doesn't know about the writer, and I'm not even sure we'll need a writer in the mock implementation.
| use crate::message::parser::RawFixMessage; | ||
|
|
||
| #[async_trait] | ||
| pub trait Actor<M: Send> { |
There was a problem hiding this comment.
This is possibly a good abstraction, but I don't see us taking advantage of it anywhere in this PR currently.
There was a problem hiding this comment.
Also, I wouldn't conflate this PR with the creation of a more generic actor framework. It requires a bit more consideration as not all actors fit this exact implementation in fn run.
|
|
||
| pub struct FixConnection { | ||
| _writer: WriterRef, | ||
| pub struct FixConnection<W: WriterModel> { |
There was a problem hiding this comment.
So this isn't on the right track - there is no need for the FixConnection to be generic. This relates to how I don't think the WriterRef needs to be further abstracted either.
| } | ||
|
|
||
| /// Spawn a TCP or TLS FIX Connection | ||
| pub async fn build_connection( |
There was a problem hiding this comment.
This being moved out of FixConnection is a good step.
No description provided.