Closed Bug 1749510 Opened 3 years ago Closed 2 years ago

Glean Rust shutdown() doesn't have a way to await upload tasks

Categories

(Data Platform and Tools :: Glean: SDK, defect, P1)

defect

Tracking

(firefox112 fixed)

RESOLVED FIXED
Tracking Status
firefox112 --- fixed

People

(Reporter: travis_, Assigned: janerik)

References

Details

Attachments

(1 file)

When the Glean Rust SDK triggers an upload it doesn't keep track of the handle of the thread that it spawns. If it did track the thread handles, then when glean::shutdown() is called these threads could be awaited. This isn't useful for desktop, so it might be necessary to have a separate shutdown() function that awaits uploads or a passed parameter to the shutdown() function to control this functionality.

For an example where I ran into this, I was adding Glean to the Nimbus CLI example to test the Rust bindings and telemetry collection and because it was a CLI app and exited quickly, Glean didn't have enough time to finish uploading pings without a sleep, because shutdown doesn't wait for uploads to complete.

This isn't useful for desktop

Really? I mean, sure, desktop doesn't care enough (at least not yet) to have asked to be able to await ping sends on shutdown...

FOG's Firefox Desktop shutdown comes after desktop net teardown meaning all these ping upload requests would quickly fail anyway, so join'ing on glean.upload wouldn't be a big deal.

An additional wrinkle is the background update agent which has its own ideas of how it wants shutdown to proceed (see bug 1703572).

All in all, I'm not sure Desktop needs a specifically distinct behaviour here. We might be just fine.

(In reply to Chris H-C :chutten from comment #1)

This isn't useful for desktop

Really? I mean, sure, desktop doesn't care enough (at least not yet) to have asked to be able to await ping sends on shutdown...

FOG's Firefox Desktop shutdown comes after desktop net teardown meaning all these ping upload requests would quickly fail anyway, so join'ing on glean.upload wouldn't be a big deal.

See, I wasn't sure anymore and had a vague feeling we blocked something by mistake which we changed.
If this "just works" for Desktop: even better!

bug 1797884 is an instance of an intermittent test failure that we suspect is likely caused by shutdown having no way to await upload tasks in progress. In it the Glean SDK uses the Glean storage for internal uploader metrics (number of pending pings, size of the pending ping dir, that sort of thing.) which could very well be happening during shutdown (where we delete the storage as we're using it).

This one's happening in a test (with test_reset_glean and all that) in much the same way as we worked around Swift test interkittens via this code... so even ignoring outside consumers like Nimbus, Firefox Desktop Background Update Agent, and Firefox Desktop Directory Cleaner, we have our own reasons for making this work (and for updating test_reset_glean to take advantage of it).

Blocks: 1797884

Kicking back to triage now that the scope's changed.

Priority: P3 → --
Whiteboard: [telemetry:glean-rs:?]

I plan to propose a way to handle this in the next week.

Assignee: nobody → jrediger
Priority: -- → P2
Blocks: 1814592

badboy merged PR #2332: "Bug 1749510 - RLB: Wait for uploader on shutdown using new shutdown mechanism" in dbc1281.

This landed. I want to take this for a spin in m-c, see if this works as expected there.

Priority: P2 → P1

This landed a while ago in a release and subsequently in m-c. Nothing suspicious popped up so I'm closing this.
We have some "See Also" bugs and those can be tackled independently.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

(In reply to Jan-Erik Rediger [:janerik] from comment #8)

This landed a while ago in a release and subsequently in m-c. Nothing suspicious popped up so I'm closing this.
We have some "See Also" bugs and those can be tackled independently.

Could you please set the tracking flags so it's clear when this hit Nightly, Beta, Release, etc? I had the same issue with https://bugzilla.mozilla.org/show_bug.cgi?id=1802094: it's not easy to track these out of tree changes and see when they make it to Firefox.

Flags: needinfo?(jrediger)

Absolutely. We indeed forgot to link the bug landing the update here as well.

Depends on: 1816454
Flags: needinfo?(jrediger)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: