Disable scheduleSend when onMessage callback is running #144

nemoshlag · 2022-11-07T15:30:33Z

Resolves #88

…ppening

codecov · 2022-11-07T15:33:15Z

Codecov Report

Patch coverage: 93.18% and project coverage change: +0.90 🎉

Comparison is base (efddaa2) 76.11% compared to head (ad3a057) 77.02%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #144      +/-   ##
==========================================
+ Coverage   76.11%   77.02%   +0.90%     
==========================================
  Files          24       24              
  Lines        1834     1893      +59     
==========================================
+ Hits         1396     1458      +62     
+ Misses        326      324       -2     
+ Partials      112      111       -1

Impacted Files	Coverage Δ
client/types/callbacks.go	`66.66% <ø> (ø)`
server/httpconnection.go	`33.33% <0.00%> (ø)`
client/internal/mockserver.go	`82.35% <85.71%> (+0.31%)`	⬆️
server/serverimpl.go	`58.92% <94.73%> (+0.37%)`	⬆️
client/httpclient.go	`95.00% <100.00%> (+0.26%)`	⬆️
client/internal/clientcommon.go	`79.80% <100.00%> (ø)`
client/internal/httpsender.go	`72.22% <100.00%> (+1.25%)`	⬆️
client/internal/receivedprocessor.go	`85.03% <100.00%> (+0.74%)`	⬆️
client/internal/sender.go	`93.33% <100.00%> (+5.33%)`	⬆️
server/callbacks.go	`68.18% <100.00%> (-4.55%)`	⬇️
... and 1 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

client/internal/receivedprocessor.go

tigrannajaryan · 2022-11-11T23:39:27Z

client/internal/receivedprocessor.go

+func (r *receivedProcessor) onMessage(ctx context.Context, msgData *types.MessageData) {
+	r.senderCommon.DisableScheduleSend()
+	r.callbacks.OnMessage(ctx, msgData)
+	r.senderCommon.EnableScheduleSend()


Nit: move this up and use defer so that if the code becomes more complicated in the future there is no chance that EnableScheduleSend() is not executed on some of the return paths.

client/internal/receivedprocessor.go

client/internal/sender.go

tigrannajaryan

Code LGTM, I just left a few comments about improving the comments (pun not intended).

tigrannajaryan · 2022-11-23T21:53:25Z

client/internal/receivedprocessor.go

 		r.callbacks.OnMessage(ctx, msgData)

+		r.sender.EnableScheduleSend()


Can we add a comment to EnableScheduleSend definition that calling EnableScheduleSend when it is already enabled is allowed? (since we do it twice here).

tigrannajaryan · 2022-11-23T21:55:03Z

client/internal/sender.go

@@ -31,6 +38,12 @@ type SenderCommon struct {
 	// Indicates that there is a pending message to send.
 	hasPendingMessage chan struct{}

+	// Indicates onMessage callback is running


Nit: the comment doesn't match what the field name implies. It would be best to reword it to something like "Set to non-zero to indicate that the message sending is disabled"

tigrannajaryan · 2022-11-23T21:55:39Z

client/internal/sender.go

 	}
 }

 // ScheduleSend signals to HTTPSender that the message in NextMessage struct
 // is now ready to be sent. If there is no pending message (e.g. the NextMessage was
 // already sent and "pending" flag is reset) then no message will be sent.
 func (h *SenderCommon) ScheduleSend() {
+	if h.IsSendingDisabled() {
+		// onMessage callback is running, ScheduleSend() will rerun after it is done


Same here, comment doesn't match what the code and func names imply.

tigrannajaryan · 2022-11-23T21:56:10Z

client/internal/sender.go

+	atomic.StoreInt32(&h.isSendingDisabled, 1)
+}
+
+// EnableScheduleSend re-enables ScheduleSend and checks if it was called during onMessage callback


Nit: I would refrain from mentioning onMessage here. It is not a concern of SenderCommon.

tigrannajaryan · 2022-11-24T15:03:36Z

Technically the change in this PR can be the cause of the test failure:

go test -race ./...
--- FAIL: TestUpdatePackages (5.10s)
    --- FAIL: TestUpdatePackages/error_on_callback (5.03s)
        --- FAIL: TestUpdatePackages/error_on_callback/ws (5.01s)
            mockserver.go:250: Time out expecting a message from the client: compressed PackageStatuses

This happened because the server did not receive a message client was supposed to send. I haven't seen this error before and it is not reproducible on the latest main.

I can't see the problem by reading the code but maybe there is some race that I am not seeing that causes a ScheduleSend to missfire?

Can you try go test -run TestUpdatePackages/error_on_callback/ws -race -count 1000 on this change and see if it is possible to reproduce?

tigrannajaryan · 2022-11-28T16:25:55Z

client/internal/sender.go

+	select {
+	case <-h.registerScheduleSend:
+		h.ScheduleSend()
+	case <-time.Tick(100 * time.Millisecond):


Unclear why this is necessary.

This fixed my sync issue. Several tests failed when testing with -race -count 1000 flags. Honestly, I don't fully understand why but it seems that the default case was selected over reading from the registerScheduleSend channel even when it (should have) held an unread message.

Failing tests may be indicating a real problem. The current code (before this PR) does not fail the tests.

The addition of the second case that waits a time without explaining why it fixes the problem is not an acceptable code change. We need precise explanation what the problem is and why this is the correct fix for it.

I've removed this bypass and tests are looking stable at the moment with the updated branch. Still, I don't have a smoking gun regarding the sync issue, but it could have been resolved due to recent commits. Would appreciate your opinion on whether or not this should stay blocked or more changes are needed

tigrannajaryan

Blocking until we have clarity around failing tests.

srikanthccv · 2023-07-01T14:17:48Z

@nemoshlag, any chance you can rebase and resolve the conflicts?

fix(ScheduleSend): disable scheduleSend when onMessage callback is ha…

a798ca2

…ppening

nemoshlag requested a review from a team November 7, 2022 15:30

nemoshlag changed the title ~~fix(ScheduleSend): disable scheduleSend when onMessage callback is ha…~~ WIP fix(ScheduleSend): disable scheduleSend when onMessage callback is ha… Nov 8, 2022

fix(ScheduleSend): fix data race with atomic wrapper

122c3f3

nemoshlag changed the title ~~WIP fix(ScheduleSend): disable scheduleSend when onMessage callback is ha…~~ Disable scheduleSend when onMessage callback is running Nov 8, 2022

Merge branch 'open-telemetry:main' into onMessageDelaySchedule

f9558fe

tigrannajaryan reviewed Nov 14, 2022

View reviewed changes

nemoshlag added 2 commits November 15, 2022 11:34

cr fix: use defer to EnableScheduleSend()

51179e6

cr fix: expose enable/disableScheduleSend via sender

063f7e4

tigrannajaryan reviewed Nov 21, 2022

View reviewed changes

client/internal/receivedprocessor.go Outdated Show resolved Hide resolved

client/internal/receivedprocessor.go Show resolved Hide resolved

client/internal/sender.go Outdated Show resolved Hide resolved

client/internal/sender.go Outdated Show resolved Hide resolved

rename onMessageRunning to isSendingDisabled

58f00eb

tigrannajaryan reviewed Nov 23, 2022

View reviewed changes

comments clarity fix

aa84a31

fix test race

2a3893d

tigrannajaryan reviewed Nov 28, 2022

View reviewed changes

tigrannajaryan requested changes Nov 30, 2022

View reviewed changes

nemoshlag added 2 commits December 12, 2022 16:43

Merge branch 'open-telemetry:main' into onMessageDelaySchedule

0bacb27

fix channel block bug

3ba2e99

nemoshlag changed the title ~~Disable scheduleSend when onMessage callback is running~~ [WIP] Disable scheduleSend when onMessage callback is running Dec 13, 2022

nemoshlag marked this pull request as draft December 13, 2022 09:33

research sync issue

28f7656

nemoshlag changed the title ~~[WIP] Disable scheduleSend when onMessage callback is running~~ Disable scheduleSend when onMessage callback is running Dec 14, 2022

nemoshlag added 2 commits December 14, 2022 15:58

research sync issue

63cb457

cleanup

62dae79

nemoshlag marked this pull request as ready for review December 14, 2022 14:04

nemoshlag requested a review from tigrannajaryan December 14, 2022 14:04

Merge branch 'main' into onMessageDelaySchedule

348739e

Merge branch 'main' into onMessageDelaySchedule

ad3a057

nemoshlag force-pushed the onMessageDelaySchedule branch from 1842cd8 to ad3a057 Compare July 6, 2023 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable scheduleSend when onMessage callback is running #144

Disable scheduleSend when onMessage callback is running #144

nemoshlag commented Nov 7, 2022 •

edited by tigrannajaryan

Loading

codecov bot commented Nov 7, 2022 •

edited

Loading

tigrannajaryan Nov 11, 2022

tigrannajaryan left a comment

tigrannajaryan Nov 23, 2022

tigrannajaryan Nov 23, 2022

tigrannajaryan Nov 23, 2022

tigrannajaryan Nov 23, 2022

tigrannajaryan commented Nov 24, 2022 •

edited

Loading

tigrannajaryan Nov 28, 2022

nemoshlag Nov 28, 2022

tigrannajaryan Nov 30, 2022

nemoshlag Dec 14, 2022

tigrannajaryan left a comment

srikanthccv commented Jul 1, 2023

		r.callbacks.OnMessage(ctx, msgData)

		r.sender.EnableScheduleSend()

Disable scheduleSend when onMessage callback is running #144

Are you sure you want to change the base?

Disable scheduleSend when onMessage callback is running #144

Conversation

nemoshlag commented Nov 7, 2022 • edited by tigrannajaryan Loading

codecov bot commented Nov 7, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

tigrannajaryan left a 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

tigrannajaryan commented Nov 24, 2022 • 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

tigrannajaryan left a comment

Choose a reason for hiding this comment

srikanthccv commented Jul 1, 2023

nemoshlag commented Nov 7, 2022 •

edited by tigrannajaryan

Loading

codecov bot commented Nov 7, 2022 •

edited

Loading

tigrannajaryan commented Nov 24, 2022 •

edited

Loading