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_two (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 9408

native/cpp/CommonCpp/grpc/ClientGetReadReactor.cpp

Loading...