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.

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

Applied a new way of socket ID generation #1076

Merged

Conversation

ethouris
Copy link
Collaborator

No description provided.

@ethouris ethouris added Priority: High Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core Impact: Significant labels Jan 21, 2020
@ethouris ethouris self-assigned this Jan 21, 2020
@maxsharabayko maxsharabayko added this to the v1.5.0 milestone Jan 21, 2020
@maxsharabayko maxsharabayko added this to Backlog in Development via automation Jan 21, 2020
@maxsharabayko maxsharabayko moved this from Backlog to Needs Review in Development Jan 21, 2020
srtcore/api.cpp Outdated Show resolved Hide resolved
srtcore/api.cpp Outdated Show resolved Hide resolved
srtcore/api.cpp Outdated
Comment on lines 468 to 473
try
{
CGuard l_idlock(m_IDLock);
ns->m_SocketID = generateSocketID();
}
catch (CUDTException& e)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't you catch the exception in the above call to generateSocketID() then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because the function that calls the newSocket function will catch it anyway.

The CUDT::newConnection OTOH will be called asynchronically as a part of the handshake process, therefore no other part will catch it.

Everything is explained in the comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OTOH you might be right in one thing - if this throws an exception, no one will clean the allocated object. I have to put everything under the try block then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, no. There would be a different kind of exception to be thrown. I need to add an interceptor.

Comment on lines +368 to +372
catch (...)
{
delete ns;
throw;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reason for catching any exception?
generateSocketID() only throws CUDTException
What are other possible exceptions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a simple passthrough - whatever exception (altho' yes, currently it's only CUDTException) is thrown, it only gets intercepted here, but then propagated further.

This instruction simply intercepts everything and complements the logical requirements of this function. It doesn't take part in any exception handling at all. If you want to put any real name here, then it would be needed to be treated as this action is specific for the exception - whereas it's not. Whatever gets through it, the handling is completely general.

It's not the matter of being strict, it's the matter of level of details that this very function focuses on.

srtcore/api.cpp Outdated Show resolved Hide resolved
Development automation moved this from Needs Review to Reviewer Approved Jan 22, 2020
@maxsharabayko maxsharabayko merged commit 4f41130 into Haivision:master Jan 23, 2020
Development automation moved this from Reviewer Approved to Done Jan 23, 2020
@mbakholdina mbakholdina modified the milestones: v1.5.0, v1.4.2 Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Priority: High Type: Maintenance Work required to maintain or clean up the code
Projects
No open projects
Development
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants