Limit writes to session file #243

eliasyishak · 2024-02-28T21:09:35Z

Fixes:

Refactor analytics session handler to use File.lastModifiedSync for _lastPing #240

Related to:

Investigate race conditions on analytics related files #166

This update makes it so that we only write to the session file only when the session id needs to be updated. Previously, we were writing with every event in order to keep track of when the last ping was so that we can keep the session alive or start a new one.

We now instead use the last modified timestamp for the session file as a stand in for when the last ping happened.

The file's format has also changed

// OLD
{"session_id": 123, "last_ping": 123}

// NEW
{"session_id": 123}

I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

See our contributor guide for general expectations for PRs.
Larger or significant changes should be discussed in an issue before creating a PR.
Contributions to our repos should follow the Dart style guide and use dart format.
Most changes should add an entry to the changelog and may need to rev the pubspec package version.
Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

github-actions · 2024-02-28T21:10:21Z

Package publishing

Package	Version	Status	Publish tag (post-merge)
package:unified_analytics	5.8.6	ready to publish	`unified_analytics-v5.8.6`
package:cli_config	0.1.2	already published at pub.dev
package:extension_discovery	2.0.1-wip	WIP (no publish necessary)
package:graphs	2.3.2-wip	WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

pkgs/unified_analytics/lib/src/session.dart

pkgs/unified_analytics/lib/src/constants.dart

christopherfujino · 2024-02-28T22:00:30Z

pkgs/unified_analytics/lib/src/initializer.dart

-      'session_id': now.millisecondsSinceEpoch,
-      'last_ping': now.millisecondsSinceEpoch,
-    }));
+    sessionFile.writeAsStringSync('${now.millisecondsSinceEpoch}');


Actually, when we discussed this yesterday, I was assuming this was the client ID, and we'd never need to re-write this. Since this is the session ID, this means we would still need to write to this when starting a new session, and another process could read from it in the middle of a write, do I have that correct?

If so, I have a few thoughts:

using last modified timestamp still seems like an improvement, and we'd at least get less race conditions

STILL using JSON is probably better, because then we would get a parse error vs. just getting the WRONG session_id

let's just hard-code the json as a string, rather than invoking the overhead of jsonEncode

maybe it's enough to catch the parse error, and then re-try 1 second later?

I agree with all of these points. Removing the last ping key should be easy enough. Also with the refactor to prevent a huge amount of write operations, I think we may be able to fall back to attempting to paraeContents again?

If we see that this is still coming up in the error logs for the race condition, we can return to setting the sessionId back to the current timestamp. Thoughts?

I think we may be able to fall back to attempting to paraeContents again?

It would still be possible for the second parse to fail, right?

If we see that this is still coming up in the error logs for the race condition, we can return to setting the sessionId back to the current timestamp.

Not sure what you mean here. Are you proposing using something else other than the current timestamp for the sessionId?

It would still be possible for the second parse to fail, right?

Yes, the parseContents method always has the possibility of failing.

Not sure what you mean here. Are you proposing using something else other than the current timestamp for the sessionId?

And what I meant here is monitoring the crash logs to see if we run into this race condition again... i would suspect that we would have a lot less instances of this error since we are now writing a lot less by checking File.lastModifiedSync method

Okay so I think i found a better solution now in my latest commit

If we fail when we attempt Session.parseContents, we will now set the session id to be the current timestamp (within the catch blocks), and then we will also pass this timestamp to the file so that whatever we have set in the error handling blocks agrees with what is persisted. How does that sound?

christopherfujino · 2024-02-28T22:02:30Z

pkgs/unified_analytics/lib/src/session.dart


  Session({
    required this.homeDirectory,
    required this.fs,
    required ErrorHandler errorHandler,
  })  : sessionFile = fs.file(p.join(
            homeDirectory.path, kDartToolDirectoryName, kSessionFileName)),
+        _sessionId = DateTime.now().millisecondsSinceEpoch,


this seems like it's "solving" the problem of LateInitializationError by assigning the variable the wrong value. Or am I misunderstanding how this works?

You're correct, this value is getting set when calling the constructor and it will eventually get replaced when we run Session.initialize. This just makes it so that we don't have to make the variable late.

It's technically not an invalid session id since later on in the code, we could be setting the this value in the file because we need a new session id if the old one had 30 mins of inactivity

we could be setting the this value in the file because we need a new session id if the old one had 30 mins of inactivity

But if it has NOT been 30 minutes of inactivity, the correct session ID would be a different value, right?

Yes that's correct, we would have to decide if that is worth it to remove the late variable

lemme take a look at this later today?

sgtm, no rush

from discussion, maybe we should just make _sessionId nullable, and allow sending a null session id. this would also help when sending errors.

Yep this makes sense with me, i also just confirmed that sending null for the session id is a valid json payload for GA4

eliasyishak · 2024-03-04T20:08:39Z

pkgs/unified_analytics/lib/src/analytics.dart

-    // Initialize the session handler with the session_id and last_ping
-    // variables by parsing the json file
+    // Initialize the session handler with the session_id
+    // by parsing the json file


As a side note, I think we should refactor the error handling class so that it doesn't depend on session data AND remove the need for the log handler

This is because:

Having session information for error reports is not necessary, this is just for maintainers of this package so we know what errors are coming up in the wild

We can also remove the need for the log handler because there is no value in locally persisting the error event on the user's computer (this log file is used for the survey handler feature, we are not using error events for targeting users)

pkgs/unified_analytics/lib/src/session.dart

christopherfujino

LGTM

eliasyishak added 3 commits February 28, 2024 15:37

Refactor to use modified dt for _lastPing

7b83ba7

Fix tests + misc dartdoc fixes

09e1680

Bump version

09caa2a

github-actions bot added the package:unified_analytics label Feb 28, 2024

eliasyishak changed the title ~~Limit read writes to session json~~ Limit writes to session file Feb 28, 2024

eliasyishak commented Feb 28, 2024

View reviewed changes

pkgs/unified_analytics/lib/src/session.dart Outdated Show resolved Hide resolved

Create sessionFile in constructor

3f7ee72

eliasyishak mentioned this pull request Feb 28, 2024

Refactor to remove late variables + clean up cyclic dependencies #241

Closed

2 tasks

eliasyishak commented Feb 28, 2024

View reviewed changes

pkgs/unified_analytics/lib/src/constants.dart Outdated Show resolved Hide resolved

Simplify lastPingDateTime

00568cb

eliasyishak requested a review from christopherfujino February 28, 2024 21:31

christopherfujino reviewed Feb 28, 2024

View reviewed changes

eliasyishak added 3 commits February 29, 2024 10:46

Revert filename + pass session id to initializer

66a9198

Use nullable field for _sessionId

60bc722

Format fix

ff2578a

eliasyishak commented Mar 4, 2024

View reviewed changes

christopherfujino reviewed Mar 6, 2024

View reviewed changes

pkgs/unified_analytics/lib/src/session.dart Outdated Show resolved Hide resolved

christopherfujino approved these changes Mar 6, 2024

View reviewed changes

Change to wip + use else block for timestamp

aac63d2

eliasyishak merged commit 24087e1 into dart-lang:main Mar 6, 2024
6 checks passed

eliasyishak deleted the limit-read-writes-to-session-json branch March 6, 2024 21:32

eliasyishak restored the limit-read-writes-to-session-json branch March 6, 2024 21:32

eliasyishak deleted the limit-read-writes-to-session-json branch March 6, 2024 21:32

eliasyishak mentioned this pull request Mar 14, 2024

[unified_analytics] Crash on flutter master branch #252

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit writes to session file #243

Limit writes to session file #243

eliasyishak commented Feb 28, 2024 •

edited

Loading

github-actions bot commented Feb 28, 2024

christopherfujino Feb 28, 2024

eliasyishak Feb 28, 2024

christopherfujino Feb 28, 2024

eliasyishak Feb 28, 2024

eliasyishak Feb 29, 2024 •

edited

Loading

christopherfujino Feb 28, 2024

eliasyishak Feb 28, 2024

eliasyishak Feb 28, 2024

christopherfujino Feb 28, 2024

eliasyishak Feb 28, 2024

christopherfujino Feb 29, 2024

eliasyishak Feb 29, 2024

christopherfujino Mar 4, 2024

eliasyishak Mar 4, 2024

eliasyishak Mar 4, 2024

christopherfujino left a comment

Limit writes to session file #243

Limit writes to session file #243

Conversation

eliasyishak commented Feb 28, 2024 • edited Loading

github-actions bot commented Feb 28, 2024

Package publishing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliasyishak Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherfujino left a comment

Choose a reason for hiding this comment

eliasyishak commented Feb 28, 2024 •

edited

Loading

eliasyishak Feb 29, 2024 •

edited

Loading