Delete two unnecessary null pointer checks for the variable “m_RPCWaitDlg”

[elfring]

Description of the Change
👀 The member function “CMainDocument::RequestRPC” contains the statement “m_RPCWaitDlg = new AsyncRPCDlg();”.
Redundant checks were still applied (despite of this implementation detail).
💭 Thus remove them.

Release Notes
Two null pointer checks for the variable “m_RPCWaitDlg” could be omitted from the implementation of the function “CMainDocument::RequestRPC”.

[elfring]

Can you accept that the avoidance of redundant pointer checks and a questionable dialogue creation should be treated as separate issues?

[AenBleidd]

@elfring, why not to fix it right here? I took a look at the code, and it won't be a big code change, but that change will affect the code that is currently changed here.
So why do we need to split this into two PRs?

[elfring]

So why do we need to split this into two PRs?

💭 Clear separation of involved development concerns

[AenBleidd]

So why do we need to split this into two PRs?

💭 Clear separation of involved development concerns

This concern was raised by you. Project maintainer asked you to improve your PR to make a fix more complete and logically correct.

[elfring]

@davidpanderson picked the opportunity up to indicate an additional change possibility which might be not so clear so far. 🤔

[AenBleidd]

@elfring, now you have a chance to make it clear and improve your PR

[elfring]

What does hinder you to keep a strict development concern separation here?

[AenBleidd]

@elfring, because the change you're requested to do will touch the same code again, thus there is no reason to have two separate PRs and instead make the real improvement, and not just the change in the code that will have no effect for the users, because there is a high chance that these particular checks are optimized by the compiler, and removed completely in the resulting code.
My personal opinion: making a requested change will take less time than the time that is already spent just for this discussion.

[CharlieFenton]

I am the author of this code, and I strongly object to this PR. Due to the asynchronous logic, race conditions can occur where CMainDocument::OnRPCComplete(CRPCFinishedEvent&) is called at any time causing CMainDocument::HandleCompletedRPC() tp destroy the dialog. In that case, if CMainDocument::RequestRPC(ASYNC_RPC_REQUEST& request, bool hasPriority) again calls m_RPCWaitDlg->Destroy(); it will cause the Manager to crash.

This code is quite complex and I put in extra NULL checks as a safety measure to guard against developers inadvertently causing such situations if they modify the code in the future, as well as to guard against unintended consequences in my own code. The cost of doing NULL checks is trivial. Good programming practice dictates always making these type of checks throughout, even when they appear to be redundant.

[CharlieFenton]

f we're going to clean up the logic here let's finish the job, and create the dialog only if it's actually used.

@davidpanderson I can't remember now why I created the dialog before it is actually needed, but I seem to remember there was a reason I felt there would be a problem trying to create it while waiting for a non-responsive asynchronous RPC, or that it would be better to only need to call ShowModal().

[CharlieFenton]

@davidpanderson I can't remember now why I created the dialog before it is actually needed

As I think about this, I suspect it was because of concerns about potential race conditions.

[CharlieFenton]

A bit of background: AsyncRPC.cpp,.h and associated code in MainDocument.cpp,.h and elsewhere were a huge hack to implement asynchronous RPCs in the Manager without requiring a near total rewrite, in order to improve UI responsiveness. Therefore this is perhaps the most fragile code in all of BOINC, and should not be modified unless needed to fix a serious bug. Even the slightest changes to this code will require extremely extensive testing.

And it certainly should not be the first code touched by someone who is not yet throughly familiar with both the BOINC code and with asynchronous programming in general.

[CharlieFenton]

I have been trying to follow the email threads and conversations leading up to this PR. If I understand correctly, @elfring was seeing frequent and persistent appearances of the dreaded "communicating with client" dialog on @elfring's computer and wanted to understand the reason and find a solution. It appears this was something specific to @elfring's computer, as we have not had many such reports.

If I understand correctly, @elfring determined the hung RPC was RPC_AUTHORIZE, (actually rpc.authorize). @davidpanderson is the expert on RPCs, but I think this checks the gui_rpc_auth.cfg file in the BOINC Data folder to ensure the user has access to it. I believe this is done before many RPCs, though it may only be done once when first needed and the result retained; I'm not sure.

If that is where the RPC communication is hanging, it probably implies the client is pretty completely failing to communicate with the Manager, and that is probably where you should be looking.

@davidpanderson can you correct me if I am mistaken and perhaps elaborate further?

[elfring]

Your feedback indicates that you are looking more for changes in other directions than the integration of a tiny source code cleanup.

💭 Mentioned concerns affect also the contribution “Mgr: Don't call get_notices RPC before previous one is processed to prevent multiple display of notices” from 2013-03-04.

[elfring]

… If I understand correctly, @elfring determined the hung RPC was RPC_AUTHORIZE, …

💭 The desired clarification for such a technical detail should be continued with the corresponding issue “BOINC Manager interface shouldn't fully block user interaction when in communicating with client dialog”.

[CharlieFenton]

I looked at the code and I was mistaken about when RPC_AUTHORIZE (actually rpc.authorize) is used. The Manager calls it only once when first connecting to the client, to confirm that the Manager is authorized to communicate with the client. This is the first RPC the Manager sends to the client, so If the client is not responding to this it means the client has never responded to the Manager. As requested, i will also post this comment to issue #4048.

[AenBleidd]

I made a detailed review of the whole RequestRPC function that is touched by this code.
From one hand, there is no reason to check if m_RPCWaitDlg is NULL or not because at the moment of this check it will never be NULL. The only case when it might be NULL is when at the moment of the new call there was not enough memory, and thus object was't be created. However, in this case exception will be thrown, and the all the code below won't be executed at all.
From the other hand, this verification for NULL doesn't add any significant additional overhead to the function. Assuming that this m_RPCWaitDlg is created as an answer on the user initiated action, we're not in a rush, and thus could take some additional CPU cycles to perform this check (however, I don't believe this check will still be present in the code after the optimization by the compiler, but anyway).
Regarding the comment of @davidpanderson to create this dialog when it's really needed, I don't think this is very important in this particular case, because we (as I mentioned above) are waiting for an answer on a user initiated action, and thus while waiting for completion of the RPC, we can create a dialog beforehands, and delete it afterwards. Of course, generally it's a good practice to create objects as close to the point of their usage as possible, but in this particular case there is no need to do that: we don't have any performance related issues in this particular piece of code.

To summarise: current code is good enough, and this particular PR (and the corresponding issue) are not needed, that is why I'm closing both of them.

[elfring]

The only case when it might be NULL is when at the moment of the new call there was not enough memory, and thus object was't be created.

Such a description should be reconsidered also according to subsequent information.

However, in this case exception will be thrown, and the all the code below won't be executed at all.

This is usual C++ software behaviour.

To summarise: current code is good enough, …

Further implementation details are waiting on better solutions as it is indicated by some open issues.

[AenBleidd]

Further implementation details are waiting on better solutions as it is indicated by some open issues.

There is no issue in this particular function, and this PR doesn't fix any issue and provides no improvements to the current software behaviour.

[elfring]

We represent different software development opinions here.

[AenBleidd]

@elfring, you're completely right.
If you have in mind an improvement that will fix existing issues or provide additional useful functionality - please go ahead and create a ticket.
As you might notice, your PR (even if applied) won't fix any issue, and will not provide any improvement.
That is why if you think that this particular function has any issues - please describe this in the ticket. You might be right, and there is something we don't see, or you're wrong because you don't have so much experience with BOINC as maintainers have.
We will never find who is right and who is wrong if you will not provide any details for your statements.

In my message before, I described why I see no issue in this particular function, and described possible cases when something might go wrong, and why I don't see that this particular PR will improve anything there. Also, I see no room for improvements in this particular function.

I don't say I'm completely right, but currently you haven't provided any information and/or details that might prove that me or other BOINC maintainers are wrong.

[elfring]

…, and will not provide any improvement.

I find also a tiny source code reduction helpful.

We will never find who is right and who is wrong …

• You indicated another agreement (in principle) because of the information “From one hand, there is no reason to check if m_RPCWaitDlg is NULL or not because at the moment of this check it will never be NULL.”.
• The information “Of course, generally it's a good practice to create objects as close to the point of their usage as possible, …” indicated potential for further collateral evolution.

[AenBleidd]

I find also a tiny source code reduction helpful.

As you were already told, we are quite limited in development resources, thus we are trying to fix existing issues and implement those improvements that directly impact our users in a positive way.
If something works for years without any issues - there is no need to change that.

That is why we will not rewrite software with the new language, with the newest standard, etc.