Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not background server in ping-pong test #29

Merged
merged 1 commit into from
Sep 8, 2022
Merged

Do not background server in ping-pong test #29

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 20 additions & 21 deletions tests/shared/src/test/scala/epollcat/TcpSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,27 +142,26 @@ class TcpSuite extends EpollcatSuite {
} yield ()
}

serverCh.bind(new InetSocketAddress(0)) *> server.background.use { _ =>
val ch = clientCh
for {
serverAddr <- serverCh.localAddress
_ <- ch.connect(serverAddr)
clientLocalAddr <- ch.localAddress
_ <- IO(
assert(clue(clientLocalAddr.asInstanceOf[InetSocketAddress].getPort()) != 0)
)
bb <- IO(ByteBuffer.wrap("ping".getBytes))
wrote <- ch.write(bb)
_ <- IO(assertEquals(bb.remaining(), 0))
_ <- IO(assertEquals(wrote, 4))
bb <- IO(ByteBuffer.allocate(4))
readed <- ch.read(bb)
_ <- IO(assertEquals(readed, 4))
_ <- IO(assertEquals(bb.remaining(), 0))
res <- IO(bb.position(0)) *> IO(decode(bb))
_ <- IO(assertEquals(res, "pong"))
} yield ()
}
val client = for {
serverAddr <- serverCh.localAddress
_ <- clientCh.connect(serverAddr)
clientLocalAddr <- clientCh.localAddress
_ <- IO(
assert(clue(clientLocalAddr.asInstanceOf[InetSocketAddress].getPort()) != 0)
)
bb <- IO(ByteBuffer.wrap("ping".getBytes))
wrote <- clientCh.write(bb)
_ <- IO(assertEquals(bb.remaining(), 0))
_ <- IO(assertEquals(wrote, 4))
bb <- IO(ByteBuffer.allocate(4))
readed <- clientCh.read(bb)
_ <- IO(assertEquals(readed, 4))
_ <- IO(assertEquals(bb.remaining(), 0))
res <- IO(bb.position(0)) *> IO(decode(bb))
_ <- IO(assertEquals(res, "pong"))
} yield ()

serverCh.bind(new InetSocketAddress(0)) *> server.both(client).void
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution is simple: instead of running the server in the background to the client (where its exceptions are ignored) instead run both concurrently with equal importance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like I am sitting exams in a concurrency course ;-)
Insert usually humble "I could be wrong. If so sorry"

I think you have the right concern but are introducing a race condition here.
The server needs to have completed initialization and be
known to be in accept before the client starts (well before it issues its connect()
but the client prologue code appears short & fast).

To check my grasshopper understanding of "*>", it says (from the cats-effedt IO API)
*>[B](that: IO[B]): IO[B] Runs the current IO, then runs the parameter, keeping its result.

This means that the serverCh.bind has run to completion and written all the way to memory? Client uses serverAddr <- serverCh.localAddress. Is there an easy way for the serverCh.localAddress`
to be passed to the client rather than globally? This would guarantee sequencing and make it evident.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, #21 is related but different. This Issue reveals the logic error in #21.

When this is fixed, #21 will go underground but still be there.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have the right concern but are introducing a race condition here.

Yes, after writing this I became worried there is a race condition here myself. Let me think through it again 😂

This means that the serverCh.bind has run to completion and written all the way to memory.

Yes, that's right. bind must complete before it continues.

Is there an easy way for the serverCh.localAddress to be passed to the client rather than globally?

Sure, we could def client(serverAddress) and write it like this maybe?

serverCh.bind(new InetSocketAddress(0)) *>
  serverCh.localAddress.flatMap { serverAddress =>
    server.both(client(serverAddress)).void
  }

But I'm not convinced this will make much difference: we are still relying on serverCh.bind to complete, because otherwise won't serverCh.localAddress just return junk to us?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are making progress but there is a mélange of concerns.

  1. You are correct serverCH.bind must complete.

  2. Using the argument, eliminates the requirement that the bind result
    must have reached possibly shared, possibly distribute memory.

  3. Unless there is some semantic that the server part of .both() happens
    before the (client) part, I think there is still a race.

    I believe that there is a complex way to take a lock on a common, shared,
    volatile variable before the .both(). Have the client aquire the lock (meaning
    that it does a synchronous wait until it gets it. The client then does a tiny (2 second?)
    delay, then begins execution. Meanwhile, the server releases the lock immediately
    before its accept. Most example are written for single-stream human speed "start server; start client"
    Here we are dealing with probably parallel machine speed.

    Much as is an eyesore, I think the economic fix is explicit .delay(). start_server.delay(10).start_client_in_parallel
    I think the cats-effect IO API has a .delay() method. Yes I know that we are trying to do async code here.
    of 10 (5?) seconds is enough

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for that detailed explanation. I understand your concerns better now, specifically about shared memory across threads. However, because the Scala Native runtime is single-threaded, this should not be a problem at this time (although in the future we will have to think hard how to make these implementations thread-safe).

3. Unless there is some semantic that the server part of .both() happens
before the (client) part, I think there is still a race.

I'm confused why this needs to be the case. The very first step in server is to accept the client socket. There is no way for this step to proceed without running the client concurrently.

epollcat/tests/shared/src/test/scala/epollcat/TcpSuite.scala

Line 133 in 9c59225

val server = serverCh.accept.use { ch =>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will study this more in the light. In another discussion you posted
the code for "local and remote addresses".

There, the server socket is definitely "listening". The same is true
here. I think there is a time window, where the client can connect
to the ServerSocket (at the indicated port) and the TCP handshake
is 'held" for a short period of time. I need to check, with a clear
head and clear mind if my understanding is right and how long
"short" is. Memory has it that it is not forever.

I think a well-defined, if ugly, and obvious happens-before leads to
lower support costs.

More later.

Thank you for chasing this.

}
}

Expand Down