Do not send if payload size > max allowed. Fixes #879 #1230
Conversation
|
Looks solid |
|
All of those failed tests seem to pass on my machine. Build server instability, or did I miss something? |
|
@cpx thanks for sending this in! Some of the tests are ones that are known to be racy, but your new test that you added has failed too. The smaller hardware we run the build server on is good at exposing race conditions that our powerful development laptops miss. Take a look at the error message and see if there's something going on with the code you added or the test you wrote, because if the test fails then it means it's sensitive to the latency from lower degrees of concurrency (crappy CPUs and less of them :p ) |
|
@Aaronontheweb Well, it fails on the exception expectation; it received 2 OversizedPayloadException events instead of one. Seems strange to me since we're only sending one message and there's only one place where the error is logged. I"m a bit stumped; any tips on how that might happen? |
| @@ -51,10 +54,11 @@ private static string GetConfigString(Config conf, string name) | |||
| return value; | |||
| } | |||
|
|
|||
| public TestTransport(Address localAddress, AssociationRegistry registry, string schemeIdentifier = "test") | |||
| public TestTransport(Address localAddress, AssociationRegistry registry, long maximumPayloadBytes = 32000, string schemeIdentifier = "test") | |||
There was a problem hiding this comment.
Does the Scala JVM transport support this constructor out of the box? Been a while since I've looked
There was a problem hiding this comment.
|
@cpx do we have an equivalent of this https://github.com/akka/akka/blob/master/akka-remote/src/main/scala/akka/remote/Endpoint.scala#L934 on the |
| Transport.MaximumPayloadBytes, | ||
| send.Message.GetType(), | ||
| pdu.Length)); | ||
| _log.Error(reason, "Transient association error (association remains live)"); |
There was a problem hiding this comment.
To get the spec to pass, you need to add return true; on a new line below here - this will signal the EndpointWriter not to queue up the oversized message for backoff + retry.
This is also equivalent to what the Scala code looks like: https://github.com/akka/akka/blob/master/akka-remote/src/main/scala/akka/remote/Endpoint.scala#L748
|
@Aaronontheweb Ah, what a silly mistake from my side. Thank you for the help! |
cpx86
force-pushed
the
879-log_frame_size_exceeding
branch
from
fae1a77 to
e17a66d
Compare
|
@cpx looks like my pre-build macro I added in Akka.MultiNodeTestRunner in my previous PR caused some problems this time. Please remove it if you don't mind :p That's an interesting point.... I think it's intentional though but I'll have to double check. Once you've fixed that build issue, could you squash all of these commits onto dev? |
- Added property MaximumPayloadBytes to Transport. For HeliosTransport this is the same as MaxFrameSize. - If incoming or outgoing payload size > maximum-payload-bytes, log OversizedPayloadException and discard message. Corresponding Akka Scala code: https://github.com/akka/akka/blob/master/akka-remote/src/main/scala/akka/remote/Endpoint.scala#L745 https://github.com/akka/akka/blob/master/akka-remote/src/main/scala/akka/remote/Endpoint.scala#L934 https://github.com/akka/akka/blob/master/akka-remote/src/main/scala/akka/remote/transport/AbstractTransportAdapter.scala#L80 https://github.com/akka/akka/blob/master/akka-remote/src/main/scala/akka/remote/transport/Transport.scala#L92 https://github.com/akka/akka/blob/master/akka-remote/src/main/scala/akka/remote/transport/netty/NettyTransport.scala#L268 https://github.com/akka/akka/blob/master/akka-remote/src/main/scala/akka/remote/transport/TestTransport.scala#L29 https://github.com/akka/akka/blob/master/akka-remote/src/test/scala/akka/remote/RemotingSpec.scala#L527
cpx86
force-pushed
the
879-log_frame_size_exceeding
branch
from
e17a66d to
05f57b9
Compare
|
Looks great - nicely done @cpx ! |
Do not send if payload size > max allowed. Fixes #879
this is the same as MaxFrameSize.
and do not send message.
Corresponding Akka Scala code:
https://github.com/akka/akka/blob/master/akka-remote/src/main/scala/akka/remote/Endpoint.scala#L745
https://github.com/akka/akka/blob/master/akka-remote/src/test/scala/akka/remote/RemotingSpec.scala#L527
https://github.com/akka/akka/search?utf8=%E2%9C%93&q=maximumpayloadbytes