-
Notifications
You must be signed in to change notification settings - Fork 150
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
Fix serialization tests & don't block thread #846
Conversation
- Fixed potential null ref when closing streams - Always clean up serialization queue if an error occurs - Stop blocking dispatcher thread by not awaiting task that processes the message - Improved error logging in EventFlowValidator to help debug issues My hope is this will result in the tests reliably passing now. Will monitor in GCI to verify
} | ||
catch | ||
{ | ||
// Do not care if there was an error removing this, must always delete if something failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must always delete if something failed [](start = 72, length = 38)
You're not actually deleting the file though - is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question. I have a gap here for sure. On consideration, I will ensure the write stream is closed out in this case and intentionally leave file contents that have been written as-is. The thinking is that for most formats (CSV, JSON) users are going to be happier with half the written contents on an unexpected error than none.
Charles pointed out that you are not actually deleting the file, looks good other than that. |
// Run in separate thread so that message thread isn't held up by a potentially time consuming file write | ||
Task.Run(async () => { | ||
await RunSerializeStartRequest(serializeParams, requestContext); | ||
}).ContinueWithOnFaulted(async t => await SendErrorAndCleanup(serializeParams?.FilePath, requestContext, t.Exception)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So a potential problem is that now since this is truly async we'll immediately return - at which point the caller may then call to continue serialization. But the start request may not actually have started/finished yet and thus we have a race condition if the continue serialization call starts trying to do stuff assuming that the start request has finished.
Same issue with the continue calls - those could then get out of order or start causing problems trying to write at the same time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this is dependent on the caller waiting on responses. That's what I've implemented in ADS - we wait on the result to come back, then send the next request. Since each one is awaited, we will get correct ordering. Right now I would treat this as "by design" because:
- the cost to implement differently would be a lot higher since you then need an ordered queue model to track
- most importantly since we're sending potentially large amounts of data, all of which are kept in memory until serialized, I explicitly do not want to encourage "send 5 messages in a row" and instead require "send message, wait until processed, send next message".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, yeah my mistake. Got confused about the return value here being the indication that the request was done. Nevermind!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest leaving a comment in the code about this stated 'by design' contract.
src/Microsoft.SqlTools.ServiceLayer/QueryExecution/SerializationService.cs
Outdated
Show resolved
Hide resolved
I suggest that leave a comment in the code about the stated ‘by design’ contract in this pull review thread.
From: Kevin Cunnane <notifications@github.com>
Sent: Thursday, August 8, 2019 3:34 PM
To: microsoft/sqltoolsservice <sqltoolsservice@noreply.github.com>
Cc: Arvind Ranasaria <Arvind.Ranasaria@microsoft.com>; Comment <comment@noreply.github.com>
Subject: Re: [microsoft/sqltoolsservice] Fix serialization tests & don't block thread (#846)
@kevcunnane commented on this pull request.
________________________________
In src/Microsoft.SqlTools.ServiceLayer/QueryExecution/SerializationService.cs<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2Fsqltoolsservice%2Fpull%2F846%23discussion_r312270178&data=02%7C01%7CArvind.Ranasaria%40microsoft.com%7Cbe5c9465db734922896208d71c5088db%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637009004522568897&sdata=%2FrvDkhdrHzSu7tVF9sdSTzhp%2BrSqHs%2BIpkfifm228nA%3D&reserved=0>:
RequestContext<SerializeDataResult> requestContext)
+ {
+ // Run in separate thread so that message thread isn't held up by a potentially time consuming file write
+ Task.Run(async () => {
+ await RunSerializeStartRequest(serializeParams, requestContext);
+ }).ContinueWithOnFaulted(async t => await SendErrorAndCleanup(serializeParams?.FilePath, requestContext, t.Exception));
So, this is dependent on the caller waiting on responses. That's what I've implemented in ADS - we wait on the result to come back, then send the next request. Since each one is awaited, we will get correct ordering. Right now I would treat this as "by design" because:
* the cost to implement differently would be a lot higher since you then need an ordered queue model to track
* most importantly since we're sending potentially large amounts of data, all of which are kept in memory until serialized, I explicitly do not want to encourage "send 5 messages in a row" and instead require "send message, wait until processed, send next message".
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2Fsqltoolsservice%2Fpull%2F846%3Femail_source%3Dnotifications%26email_token%3DAJ5JMVRHI6SZA4FQ6COKX3LQDSNOFA5CNFSM4IKO4QHKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBB3BNY%23discussion_r312270178&data=02%7C01%7CArvind.Ranasaria%40microsoft.com%7Cbe5c9465db734922896208d71c5088db%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637009004522568897&sdata=7H0bEb%2BjSuBNNs9VIj0vMTwkO0xgPDxoWKu5%2Fmku3LE%3D&reserved=0>, or mute the thread<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAJ5JMVU734YY3O55QQMVXQTQDSNOFANCNFSM4IKO4QHA&data=02%7C01%7CArvind.Ranasaria%40microsoft.com%7Cbe5c9465db734922896208d71c5088db%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637009004522578898&sdata=U2BCnDkc8NIZgthukT%2FuNAuYIUgj1gZuvV3w4EIC0Co%3D&reserved=0>.
|
My hope is this will result in the tests reliably passing now.
Will monitor in GCI to verify