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.

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

Ensure span cancellation is done properly with concurrency protection #217

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

kstenerud
Copy link
Contributor

@kstenerud kstenerud commented Nov 21, 2023

Goal

Using networkRequestCallback_ null value as a flag affecting behaviour is too fragile and has led to multiple bugs. Refactor this section to just re-run the callback for the (max 10 or so) initial spans instead, which makes the concurrency checks far less complex / prone to holes.

As a result:

  • isEarlySpansPhase_ and dependent code will never suffer race conditions
  • Spans will never end up partially initialized because they ran in between states where the network callback pointed to actual user code.
  • The callbacks can never be null

Copy link

github-actions bot commented Nov 21, 2023

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +2.6% +3.98Ki  +2.6% +3.98Ki    String Table
  +1.2%   +1008  +1.2%   +1008    Symbol Table
  +5.2%    +224  +5.2%    +224    __DATA,__const
  +6.2%    +164  +6.2%    +164    __TEXT,__const
  +0.1%    +116  +0.1%    +116    __TEXT,__text
  +1.6%     +85  +1.6%     +85    __TEXT,__cstring
  +1.2%     +44  +1.2%     +44    __TEXT,__unwind_info
  +1.8%     +16  +1.8%     +16    Function Start Addresses
  +0.2%      +8  +0.2%      +8    Binding Info
  +1.1%      +8  +1.1%      +8    Rebase Info
  -0.0%      -1  -0.0%      -1    __TEXT,__objc_methname
  -0.2%     -16  -0.2%     -16    __TEXT,__gcc_except_tab
  -4.3%    -224  -4.4%    -224    [__DATA]
  -1.1%    -392  -1.1%    -392    [__TEXT]
  [ = ]       0 -33.2% -4.99Ki    [__LINKEDIT]
  +1.1% +4.99Ki  [ = ]       0    TOTAL

Generated by 🚫 Danger

@kstenerud kstenerud force-pushed the karl-span-cancellation-fix branch 2 times, most recently from 55c98e4 to a77c86d Compare November 21, 2023 14:43
@kstenerud kstenerud force-pushed the karl-span-cancellation-fix branch from a77c86d to 7346dde Compare November 21, 2023 15:21
@kstenerud kstenerud marked this pull request as ready for review November 21, 2023 15:33
@kstenerud kstenerud merged commit cdca9ca into next Nov 22, 2023
1 check passed
@kstenerud kstenerud deleted the karl-span-cancellation-fix branch November 22, 2023 09:25
@kstenerud kstenerud mentioned this pull request Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants