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.

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

Breaks out of the loop if the SCTP socket early #226

Closed
wants to merge 1 commit into
base: master
from
Jump to file or symbol
Failed to load files and symbols.
+3 −3
Diff settings

Always

Just for now

@@ -1080,6 +1080,9 @@ public void run()
{
do
{
if (sctpSocket == null)
break;

This comment has been minimized.

@lyubomir

lyubomir Apr 20, 2016

Member

Could I please ask you to write a comment explaining why the check's there? I got the reason from your PR comment but I don't think that's obvious (to me).

@lyubomir

lyubomir Apr 20, 2016

Member

Could I please ask you to write a comment explaining why the check's there? I got the reason from your PR comment but I don't think that's obvious (to me).

iceSocket.receive(recv);
RawPacket[] send
@@ -1118,9 +1121,6 @@ public void run()
}
}
if (sctpSocket == null)
break;

This comment has been minimized.

@lyubomir

lyubomir Apr 20, 2016

Member

I get the reason why the same check is necessary before the receive (after reading your PR comment). But I don't get why it's no longer necessary after the receive. Generally, a receive usually takes significantly long time to return so checks from before may not hold afterwards. In the case that may or may not be true, I'm just saying that it's not obvious to me.

On the other hand, the for loop bellow will repeatedly access sctpSocket so it's not like it will not become null inside the loop (I guess).

Anyway, I suppose I mostly wanted to express my feeling that something's amiss around these null checks outside a loop and accesses to a field in a loop.

It's outside the scope of this PR, of course, but I really feel that this method should be broken down into multiple smaller ones in order to aid reading and understanding.

@lyubomir

lyubomir Apr 20, 2016

Member

I get the reason why the same check is necessary before the receive (after reading your PR comment). But I don't get why it's no longer necessary after the receive. Generally, a receive usually takes significantly long time to return so checks from before may not hold afterwards. In the case that may or may not be true, I'm just saying that it's not obvious to me.

On the other hand, the for loop bellow will repeatedly access sctpSocket so it's not like it will not become null inside the loop (I guess).

Anyway, I suppose I mostly wanted to express my feeling that something's amiss around these null checks outside a loop and accesses to a field in a loop.

It's outside the scope of this PR, of course, but I really feel that this method should be broken down into multiple smaller ones in order to aid reading and understanding.

This comment has been minimized.

@bgrozev

bgrozev Apr 20, 2016

Member

I will close this PR. I agree that we should refactor this anyway, and doing so is on the roadmap, so I'll leave it for now.

@bgrozev

bgrozev Apr 20, 2016

Member

I will close this PR. I agree that we should refactor this anyway, and doing so is on the roadmap, so I'll leave it for now.

This comment has been minimized.

@lyubomir

lyubomir Apr 20, 2016

Member

I'm sorry.

@lyubomir

lyubomir Apr 20, 2016

Member

I'm sorry.

// Pass network packet to SCTP stack
for (RawPacket s : send)
{
ProTip! Use n and p to navigate between commits in a pull request.