Skip to content
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.

Sign up for GitHub

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

Jump to bottom

audit background contexts #14869

Merged
merged 1 commit into from
Nov 6, 2024
Merged

audit background contexts #14869

merged 1 commit into from
Nov 6, 2024

Conversation

jmank88
Copy link
Contributor

@jmank88 jmank88 commented Oct 21, 2024

@jmank88 jmank88 mentioned this pull request Oct 21, 2024
Merged
@github-actions GitHub Actions
Copy link
Contributor

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

  • #added For any new functionality added.
  • #breaking_change For any functionality that requires manual action for the node to boot.
  • #bugfix For bug fixes.
  • #changed For any change to the existing functionality.
  • #db_update For any feature that introduces updates to database schema.
  • #deprecation_notice For any upcoming deprecation functionality.
  • #internal For changesets that need to be excluded from the final changelog.
  • #nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
  • #removed For any functionality/config that is removed.
  • #updated For any functionality that is updated.
  • #wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 21, 2024 •
edited
Loading

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@jmank88 jmank88 force-pushed the BCF-3204-background-ctx branch 5 times, most recently from 8601834 to d1a62bc Compare October 22, 2024 15:16
@jmank88 jmank88 force-pushed the BCF-3204-background-ctx branch 5 times, most recently from cf797c9 to 9e12028 Compare October 24, 2024 20:16
@jmank88 jmank88 marked this pull request as ready for review October 24, 2024 20:21
@jmank88 jmank88 requested review from a team as code owners October 24, 2024 20:21
@jmank88 jmank88 requested a review from ilija42 November 5, 2024 14:25
EasterTheBunny
EasterTheBunny approved these changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@EasterTheBunny EasterTheBunny left a comment

Choose a reason for hiding this comment

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

LGTM

samsondav
samsondav approved these changes Nov 5, 2024
View reviewed changes
Copy link
Collaborator

@samsondav samsondav left a comment

Choose a reason for hiding this comment

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

Mercury stuff looks good

infiloop2
infiloop2 reviewed Nov 6, 2024
View reviewed changes
core/services/ocr2/plugins/ocr2keeper/evmregistry/v21/mercury/v03/request.go
@@ -74,11 +74,10 @@ func (c *client) DoRequest(ctx context.Context, streamsLookup *mercury.StreamsLo
c.multiFeedsRequest(ctx, ch, streamsLookup)
})

// TODO (AUTO 9090): Understand and fix the use of context.Background() here
reqTimeoutCtx, cancel := context.WithTimeout(context.Background(), mercury.RequestTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

We had actually seen strange issues when we were creating a new context from existing context with timeout instead of context.background where we saw the timeout was not being respected. https://smartcontract-it.atlassian.net/browse/AUTO-9090 has more details

if we want to fix this ideally we should also test the behaviour to ensure it is working correctly. I am not sure what's the impact of letting this code be as is, is there a possibility to have an exception for use of background context here and not change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand the issue described in the ticket. If the claim is that something continues to run longer than the given timeout, that is not necessarily a bug, and leaving out the parent context here will have no effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, the timeout is not being applied to the actual request, only below when waiting for the response. That seems incorrect. Should this be moved up before the threadCtrl.GoCtx call?

@jmank88 jmank88 added this pull request to the merge queue Nov 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 6, 2024
@jmank88 jmank88 added this pull request to the merge queue Nov 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 6, 2024
@jmank88 jmank88 added this pull request to the merge queue Nov 6, 2024
Merged via the queue into develop with commit 9e899bb Nov 6, 2024
162 of 164 checks passed
@jmank88 jmank88 deleted the BCF-3204-background-ctx branch November 6, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@infiloop2 infiloop2 infiloop2 left review comments

@0xAustinWang 0xAustinWang 0xAustinWang approved these changes

@AnieeG AnieeG AnieeG approved these changes

@eutopian eutopian eutopian approved these changes

@krehermann krehermann krehermann approved these changes

@EasterTheBunny EasterTheBunny EasterTheBunny approved these changes

@samsondav samsondav samsondav approved these changes

@MStreet3 MStreet3 Awaiting requested review from MStreet3

@cedric-cordenier cedric-cordenier Awaiting requested review from cedric-cordenier

@winder winder Awaiting requested review from winder winder is a code owner automatically assigned from smartcontractkit/ccip-offchain

@rstout rstout Awaiting requested review from rstout rstout is a code owner automatically assigned from smartcontractkit/ccip-offchain

@asoliman92 asoliman92 Awaiting requested review from asoliman92 asoliman92 is a code owner automatically assigned from smartcontractkit/ccip-offchain

@makramkd makramkd Awaiting requested review from makramkd makramkd is a code owner automatically assigned from smartcontractkit/ccip-offchain

@dimkouv dimkouv Awaiting requested review from dimkouv dimkouv is a code owner automatically assigned from smartcontractkit/ccip-offchain

@mateusz-sekara mateusz-sekara Awaiting requested review from mateusz-sekara mateusz-sekara is a code owner automatically assigned from smartcontractkit/ccip-offchain

@0xnogo 0xnogo Awaiting requested review from 0xnogo 0xnogo is a code owner automatically assigned from smartcontractkit/ccip-offchain

@chainchad chainchad Awaiting requested review from chainchad

@patrickhuie19 patrickhuie19 Awaiting requested review from patrickhuie19

@archseer archseer Awaiting requested review from archseer

@bolekk bolekk Awaiting requested review from bolekk

@connorwstein connorwstein Awaiting requested review from connorwstein

@Atrax1 Atrax1 Awaiting requested review from Atrax1

@aalu1418 aalu1418 Awaiting requested review from aalu1418

@kalverra kalverra Awaiting requested review from kalverra

@poopoothegorilla poopoothegorilla Awaiting requested review from poopoothegorilla

@DylanTinianov DylanTinianov Awaiting requested review from DylanTinianov

@dimriou dimriou Awaiting requested review from dimriou

@ettec ettec Awaiting requested review from ettec

@nolag nolag Awaiting requested review from nolag

@patrick-dowell patrick-dowell Awaiting requested review from patrick-dowell

@vyzaldysanchez vyzaldysanchez Awaiting requested review from vyzaldysanchez

@george-dorin george-dorin Awaiting requested review from george-dorin

@reductionista reductionista Awaiting requested review from reductionista

@RyanRHall RyanRHall Awaiting requested review from RyanRHall

@HenryNguyen5 HenryNguyen5 Awaiting requested review from HenryNguyen5

@momentmaker momentmaker Awaiting requested review from momentmaker

@cfal cfal Awaiting requested review from cfal

@ilija42 ilija42 Awaiting requested review from ilija42

@prashantkumar1982 prashantkumar1982 Awaiting requested review from prashantkumar1982

@obelisk obelisk Awaiting requested review from obelisk

@DeividasK DeividasK Awaiting requested review from DeividasK

@skudasov skudasov Awaiting requested review from skudasov

@smickovskid smickovskid Awaiting requested review from smickovskid

@ibrajer ibrajer Awaiting requested review from ibrajer

@shileiwill shileiwill Awaiting requested review from shileiwill

@agparadiso agparadiso Awaiting requested review from agparadiso

@anirudhwarrier anirudhwarrier Awaiting requested review from anirudhwarrier

@leeyikjiun leeyikjiun Awaiting requested review from leeyikjiun

@FelixFan1992 FelixFan1992 Awaiting requested review from FelixFan1992

@justinkaseman justinkaseman Awaiting requested review from justinkaseman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jmank88 @samsondav @EasterTheBunny @krehermann @eutopian @infiloop2 @AnieeG @chainchad @0xAustinWang