Paths

Table of Contentst

Differential D3120

[CommCoreModule] Set `readyState` to `SocketStatus::CLOSING` on call to `ClientGetReadReactor::close()`
ClosedPublic
Actions

Authored by atul on Feb 7 2022, 5:07 AM.

Tags

None

Referenced Files

	Unknown Object (File)
	Wed, Jan 8, 4:14 PM

	Unknown Object (File)
	Wed, Jan 8, 4:14 PM

	Unknown Object (File)
	Wed, Jan 8, 4:13 PM

	Unknown Object (File)
	Wed, Jan 8, 4:13 PM

	Unknown Object (File)
	Wed, Jan 8, 10:58 AM

	Unknown Object (File)
	Thu, Jan 2, 2:44 AM

	Unknown Object (File)
	Thu, Jan 2, 2:44 AM

	Unknown Object (File)
	Thu, Jan 2, 2:44 AM

Subscribers

Details

Reviewers

ashoat
tomek
• jacek
varun
• karol
• jim

Commits

rCOMMa1375e4f3d07: [CommCoreModule] Set `readyState` to `SocketStatus::CLOSING` on call to…

Summary

We don't really distinguish between 2: CLOSING and 3: CLOSED in lib/socket.react.js, but don't think it hurts to add this here.

Test Plan

Logged readyState

Diff Detail

Repository

Branch

landfeb8 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Feb 7 2022, 5:07 AM

Herald added subscribers: • benschac, • adrian. · View Herald TranscriptFeb 7 2022, 5:07 AM

Harbormaster completed remote builds in B6585: Diff 9333.Feb 7 2022, 5:12 AM

atul requested review of this revision.Feb 7 2022, 5:12 AM

• karol accepted this revision.Feb 7 2022, 7:22 AM

• karol added inline comments.

native/cpp/CommonCpp/grpc/ClientGetReadReactor.cpp
25–27 ↗	(On Diff #9333)	Should this mutex also stay locked for the `TryCancel` call?

This revision is now accepted and ready to land.Feb 7 2022, 7:22 AM

Herald added 1 blocking reviewer(s): ashoat. · View Herald TranscriptFeb 7 2022, 7:22 AM

This revision now requires review to proceed.Feb 7 2022, 7:22 AM

atul added inline comments.Feb 7 2022, 8:58 AM

native/cpp/CommonCpp/grpc/ClientGetReadReactor.cpp
25–27 ↗	(On Diff #9333)	You're right, this could cause issues. I was setting `readyState` before the call to `TryCancel` to ensure that `readyState` would be set to `2: CLOSING` before `3: CLOSED`. However, the mutex stays locked during the `TryCancel` call, and if `TryCancel` is blocked on `OnDone` (I really doubt that it is, but I haven't explicitly checked), we would have a deadlock. Since we never consider the distinction between `2: CLOSING` and `3: CLOSED`, it seems like the safest thing to do is just get rid of this diff?

ashoat requested changes to this revision.Feb 7 2022, 7:29 PM

ashoat added a reviewer: • jim.

ashoat added inline comments.

native/cpp/CommonCpp/grpc/ClientGetReadReactor.cpp
25–27 ↗	(On Diff #9333)	You can just wrap it in a block

This revision now requires changes to proceed.Feb 7 2022, 7:29 PM

rebase before addressing feedback

Harbormaster completed remote builds in B6624: Diff 9389.Feb 8 2022, 5:52 AM

address feedback

Harbormaster completed remote builds in B6625: Diff 9390.Feb 8 2022, 5:59 AM

ashoat accepted this revision.Feb 8 2022, 11:08 AM

This revision is now accepted and ready to land.Feb 8 2022, 11:08 AM

rebase before landing

trigger services CI

Harbormaster failed remote builds in B6644: Diff 9408!Feb 8 2022, 11:19 AM

Harbormaster completed remote builds in B6643: Diff 9407.Feb 8 2022, 11:23 AM

This revision was landed with ongoing or failed builds.Feb 8 2022, 11:31 AM

Closed by commit rCOMMa1375e4f3d07: [CommCoreModule] Set `readyState` to `SocketStatus::CLOSING` on call to… (authored by atul). · Explain Why

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMMa1375e4f3d07: [CommCoreModule] Set `readyState` to `SocketStatus::CLOSING` on call to….

Revision Contents
Changeset List

Path

Size

native/

cpp/

CommonCpp/

grpc/

ClientGetReadReactor.cpp

4 lines

Diff 9390

native/cpp/CommonCpp/grpc/ClientGetReadReactor.cpp

Loading...