remoting: Handle DLLImport failure of libpsrpclient #5622
Conversation
|
@mirichmo and @PaulHigin will either of you be able to look at this? |
There was a problem hiding this comment.
This field is different from array "_syncObjects" only by last letter - it is confusing.
It would be good to either add comment for "_syncObjects" array about what it is; or rename "_syncObject"; or both.
There was a problem hiding this comment.
@anmenaga: Actually, after reviewing this logic again, _syncObject is insufficient since it only guards Clear(). Additionally, _syncObjects also appears to be fragile; it guards against individual usage of a given element but assumes the elements are valid; which is the actual problem.
My thinking is _syncObjects should be removed and _syncObject used to serialize access to the set instead of individual members.
Thoughts?
@SteveL-MSFT, @daxian-dbw, @TravisEz13 Guys, would you take a look at this class. My inclincation is to remove the individual locks and lock the set.
There was a problem hiding this comment.
@dantraMSFT I'm all for simplification, but there should be a reason why they decided to go more complex 'individual-sync-object' route in the first place... Maybe something will deadlock with a single syncObject, and parallel "Add"s won't be able to execute for different DataPriorityTypes (not sure how critical this really is).
There was a problem hiding this comment.
Actually, after reviewing this logic again, _syncObject is insufficient since it only guards Clear().
_dataSyncObjects and _dataToBeSent are used in multiple methods, but it appears that there is an assumption that those methods won't be called until the Fragmentor property is set (where both _dataSyncObjects and _dataToBeSent would be initialized). With this assumption, I think it's fine to directly use _dataSyncObjects and _dataToBeSent in methods other than Clear(). But we probably should add a comment about that assumption.
For Clear(), it's possible to be called before Fragmentor being set (in error path), so it's necessary to check _dataSyncObjects for null beforehand.
|
also related to issue #5566 |
|
LGTM, but I would like @PaulHigin to have a look. |
|
@anmenaga Do you want to sign off, or did you just comment? |
|
@TravisEz13 |
|
@TravisEz13 This should be ready to merge. |
|
I've been asked to wait for a review by @PaulHigin or @daxian-dbw |
There was a problem hiding this comment.
This assert should be removed because apparently _dataToBeSent can be null in that error path.
There was a problem hiding this comment.
Never mind this comment. It's certainly right to assume _dataToBeSent != null when null != _dataSyncObjects 😄
There was a problem hiding this comment.
I think we can avoid the sync object _syncObject by the following code:
if (null != _dataSyncObjects)
{
int promptResponseIndex = (int)DataPriorityType.PromptResponse;
int defaultIndex = (int)DataPriorityType.Default;
if (_dataToBeSent[promptResponseIndex] != null)
{
lock (_dataSyncObjects[promptResponseIndex])
{
if (_dataToBeSent[promptResponseIndex] != null)
{
_dataToBeSent[promptResponseIndex].Dispose();
_dataToBeSent[promptResponseIndex] = null;
}
}
}
if (_dataToBeSent[defaultIndex] != null)
{
lock (_dataSyncObjects[defaultIndex])
{
if (_dataToBeSent[defaultIndex] != null)
{
_dataToBeSent[defaultIndex].Dispose();
_dataToBeSent[defaultIndex] = null;
}
}
}
}There was a problem hiding this comment.
You're right; I'll make the change.
…WSManAPIDataCommon.ctor was not identifying the binary that failed DLLImport and PrioritySendDataCollection.Clear() would throw a NullReferenceException when called from the finalizer after an error path. This change addresses both of the above but does not address the DllImport failure. * Guard against null arrays in PrioritySendDataCollection.Clear() - called from finalizer in error paths. * Diagnosability: Log DllNotFoundException in WSManAPIDataCommon ctor and also include as the internal exception when throwing PSRemotingTransportException.
|
@daxian-dbw do you have any additional feedback? |
|
@PaulHigin, @daxian-dbw, please take a look at this; it's marked as GA. Thanks. |
| lock (_syncObjects[(int)DataPriorityType.Default]) | ||
| { | ||
| _dataToBeSent[(int)DataPriorityType.Default].Dispose(); | ||
| lock (_dataSyncObjects[promptResponseIndex]) |
There was a problem hiding this comment.
Sorry for the late review. I am not familiar with the error path but if _dataToBeSent array can contain null entries then it seems like so can _dataSyncObjects (_dataSyncObjects[promptResponseIndex] == null ?).
It seems like these arrays should be created in the PrioritySendDataCollection constructor...
There was a problem hiding this comment.
@PaulHigin: _dataSyncObjects is populated when it is created. The entries are not null if the array is not null.
As far as when it is created, it appears the intent was to create both arrays at the same point. I saw no strong reason to change it.
There was a problem hiding this comment.
But the _dataSyncObjects array is filled within a loop:
for (int i = 0; i < names.Length; i++)
{
_dataToBeSent[i] = new SerializedDataStream(_fragmentor.FragmentSize);
_dataSyncObjects[i] = new object();
}
If an exception is thrown within the loop then it is possible an array element is null. Is this not the error path we are worried about?
There was a problem hiding this comment.
SerializedDataStream's ctor does not currently throw.
Note that it should (it relies on Dbg.Assert) and I opened an issue for this: #5800
Issue PowerShell#4029 exposed two problems when failing to load libpsrpclient. WSManAPIDataCommon.ctor was not identifying the binary that failed DLLImport; hindering diagnosability PrioritySendDataCollection.Clear() would throw a NullReferenceException when called from the finalizer after an error path. This change addresses both of the above: Guard against null arrays in PrioritySendDataCollection.Clear() - called from finalizer in error paths. Diagnosability: Log the DllNotFoundException in WSManAPIDataCommon ctor and also include it as the internal exception when throwing PSRemotingTransportException.
Issue #4029 exposed two problems when failing to load libpsrpclient. WSManAPIDataCommon.ctor was not identifying the binary that failed DLLImport; hindering diagnosability PrioritySendDataCollection.Clear() would throw a NullReferenceException when called from the finalizer after an error path. This change addresses both of the above: Guard against null arrays in PrioritySendDataCollection.Clear() - called from finalizer in error paths. Diagnosability: Log the DllNotFoundException in WSManAPIDataCommon ctor and also include it as the internal exception when throwing PSRemotingTransportException.
Issue #4029 exposed two problems when failing to load libpsrpclient.
This change addresses both of the above:
Guard against null arrays in PrioritySendDataCollection.Clear() - called from finalizer in error paths.
Diagnosability: Log the DllNotFoundException in WSManAPIDataCommon ctor and also include it as the internal exception when throwing PSRemotingTransportException.