Skip to content

Conversation

@tombogle
Copy link
Contributor

@tombogle tombogle commented Aug 11, 2022

Also upgraded to latest version of Segment's Analytics package
Misc code cleanup in Analytics.cs


This change is Reviewable

Also upgraded to latest version of Segment's Analytics package
Misc code cleanup in Analytics.cs
@tombogle tombogle force-pushed the prevent-flush-deadlock branch from e6e757a to d4e2002 Compare August 11, 2022 18:14
@tombogle
Copy link
Contributor Author

This "fix" is a kludge. I have reported the problem to Segment in hopes of a fix: segmentio/Analytics.NET#200

Copy link
Collaborator

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jasonleenaylor and @tombogle)


SampleAppWithForm/Form1.cs line 31 at r1 (raw file):

				$"Submitted: {Segment.Analytics.Client.Statistics.Submitted}; " +
				$"Failed:  {Segment.Analytics.Client.Statistics.Failed}");
			// This allows us to illustrate the deadlock problem.

Want to reference an issue and/or the code used to work around it?


src/DesktopAnalytics/Analytics.cs line 578 at r1 (raw file):

				// So instead of calling Flush, if there are events in the queue, we just wait a little while.
				// The default timeout on the client is 5 seconds, so probably we should never need to wait
				// longer than that.

Want to reference the reported issue here for future reference?


src/DesktopAnalytics/Analytics.cs line 586 at r1 (raw file):

						break;
					totalWait += 2500;
					Thread.Sleep(2500);

2.5 seconds seems like a long time to wait (I assume the user experiences this as the app taking a long time to close?)


src/DesktopAnalytics/Analytics.cs line 636 at r1 (raw file):

				// helpful if someone starts using an app built before the OS is released. Anything that
				// your app has a manifest which says it supports the OS it is running on.  This is not
				// After Windows 8 the Environment.OSVersion started misreporting information unless

This comment got messed up.

@tombogle tombogle marked this pull request as draft August 11, 2022 18:28
@jasonleenaylor
Copy link
Collaborator

src/DesktopAnalytics/Analytics.cs line 586 at r1 (raw file):

Previously, andrew-polk wrote…

2.5 seconds seems like a long time to wait (I assume the user experiences this as the app taking a long time to close?)

It seems like a long time, but since it will only happen if the app is closed when events are queued it might be fine.

@tombogle
Copy link
Contributor Author

src/DesktopAnalytics/Analytics.cs line 586 at r1 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

It seems like a long time, but since it will only happen if the app is closed when events are queued it might be fine.

Added a comment to explain this. (After more experimentation, I determined that it was occasionally possible to end up with a single wait cycle of only a second, so I just decided to make it simple and wait a half second at a time. But most of the time it takes 2-4 seconds.)

Copy link
Contributor Author

@tombogle tombogle left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 11 files reviewed, 4 unresolved discussions (waiting on @andrew-polk and @jasonleenaylor)


src/DesktopAnalytics/Analytics.cs line 578 at r1 (raw file):

Previously, andrew-polk wrote…

Want to reference the reported issue here for future reference?

Done.


src/DesktopAnalytics/Analytics.cs line 636 at r1 (raw file):

Previously, andrew-polk wrote…

This comment got messed up.

Fixed


SampleAppWithForm/Form1.cs line 31 at r1 (raw file):

Previously, andrew-polk wrote…

Want to reference an issue and/or the code used to work around it?

Done.

@tombogle tombogle marked this pull request as ready for review August 11, 2022 19:18
Copy link
Collaborator

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 7 files at r1, 6 of 7 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @andrew-polk)

Copy link
Collaborator

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @tombogle)

@tombogle tombogle merged commit 902dce2 into master Aug 12, 2022
@tombogle tombogle deleted the prevent-flush-deadlock branch August 12, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug dependencies Pull requests that update a dependency file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants