Fix isDone() handling to consider the onFailed/onCancel state of the streams. [chromium/src : main]
Mohannad Farrag (Gerrit)
Mohannad Farrag voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Erik Ovelius
- Etienne Dechamps
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohannad Farrag (Gerrit)
Mohannad Farrag voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mohannad Farrag
| Code-Review | +1 |
1 comment
verify(mMockCallback, never()).onCanceled(any(), any());`assertFalse(mAdaptiveStream.isDone());`?
Related details
- Erik Ovelius
- Etienne Dechamps
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Erik Ovelius (Gerrit)
Erik Ovelius added 1 comment![]( "Open in Gerrit")
verify(mMockCallback, never()).onCanceled(any(), any());Erik Ovelius`assertFalse(mAdaptiveStream.isDone());`?
Thanks for catching!
Related details
- Etienne Dechamps
- Mohannad Farrag
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Dechamps (Gerrit)
Etienne Dechamps added 4 comments![]( "Open in Gerrit")
Withholding +1 and deferring to Mohannad to review the business logic as I have zero context on this code.
Fix isDone() handling to consider the onFailed/onCancel state of the streams. Please fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Please remove the trailing whitespace.
mBackendCallback.onFailed(
CronetAdaptiveNetworkBidirectionalStream.this, info, error);
return;
}
// Either the primary stream just failed and we have no fallback
// or we are the second stream to fail.
// Time to give up and signal failure.
if (mFallbackStream == null || mOnlyOneStreamRemains.getAndSet(true)) {
mBackendCallback.onFailed(
CronetAdaptiveNetworkBidirectionalStream.this, info, error);
return;
}
}
@Override
public void onCanceled(BidirectionalStream stream, UrlResponseInfo info) {
checkValidStream(stream);
// The active stream was cancelled failed.
if (mActiveStream.get() == stream) {
mBackendCallback.onCanceled(
CronetAdaptiveNetworkBidirectionalStream.this, info);
return;
}
// Either the primary stream just cancelled and we have no fallback
// or we are the second stream to cancel.
// Signal the cancel.
if (mFallbackStream == null || mOnlyOneStreamRemains.getAndSet(true)) {
mBackendCallback.onCanceled(
CronetAdaptiveNetworkBidirectionalStream.this, info);
return;
}Spurious diffs.
if (mFallbackStream == null) {
return mPrimaryStream.isDone();
}
return mPrimaryStream.isDone() && mFallbackStream.isDone();I think the following would be easier to read:
```
bool fallbackIsDone = mFallbackStream == null ? true : mFallbackStream.isDone();
return mPrimaryStream.isDone() && fallbackIsDone;
```
Or even:
```
return mPrimaryStream.isDone() && (mFallbackStream == null ? true : mFallbackStream.isDone());
```
Or even:
```
return (mActiveStream.get() != null) ? mActiveStream.get().isDone() : (mPrimaryStream.isDone() && (mFallbackStream == null ? true : mFallbackStream.isDone()));
```
Related details
- Erik Ovelius
- Mohannad Farrag
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohannad Farrag (Gerrit)
Mohannad Farrag added 1 comment![]( "Open in Gerrit")
if (mFallbackStream == null) {
return mPrimaryStream.isDone();
}
return mPrimaryStream.isDone() && mFallbackStream.isDone();I think the following would be easier to read:
```
bool fallbackIsDone = mFallbackStream == null ? true : mFallbackStream.isDone();
return mPrimaryStream.isDone() && fallbackIsDone;
```Or even:
```
return mPrimaryStream.isDone() && (mFallbackStream == null ? true : mFallbackStream.isDone());
```Or even:
```
return (mActiveStream.get() != null) ? mActiveStream.get().isDone() : (mPrimaryStream.isDone() && (mFallbackStream == null ? true : mFallbackStream.isDone()));
```
Now that you write it that way, I think there's a little bit more complication here. If the `mActiveStream` has successfully finished the request with a terminal callback of `onSucceeded` and the `mFallback` stream exists, then the result of `isDone()` should be true.
I always get confused about this thinking that the `fallback` stream would fire if the `primary` fails in a terminal callback, but that's not true.
Can't we just do `mActiveStream.get() != null ? mActiveStream.isDone() : false`? So if there's no active stream then that means either:
1) The request did not start yet.
2) The request started but has not reached `onStreamReady`
If there's an active stream then it won't change in the future regardless of the outcome so just use `isDone()` on it.
Related details
- Erik Ovelius
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Erik Ovelius (Gerrit)
Erik Ovelius added 4 comments![]( "Open in Gerrit")
Withholding +1 and deferring to Mohannad to review the business logic as I have zero context on this code.
I think he did +1 already 😊
Fix isDone() handling to consider the onFailed/onCancel state of the streams. Please fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Please remove the trailing whitespace.
Done
mBackendCallback.onFailed(
CronetAdaptiveNetworkBidirectionalStream.this, info, error);
return;
}
// Either the primary stream just failed and we have no fallback
// or we are the second stream to fail.
// Time to give up and signal failure.
if (mFallbackStream == null || mOnlyOneStreamRemains.getAndSet(true)) {
mBackendCallback.onFailed(
CronetAdaptiveNetworkBidirectionalStream.this, info, error);
return;
}
}
@Override
public void onCanceled(BidirectionalStream stream, UrlResponseInfo info) {
checkValidStream(stream);
// The active stream was cancelled failed.
if (mActiveStream.get() == stream) {
mBackendCallback.onCanceled(
CronetAdaptiveNetworkBidirectionalStream.this, info);
return;
}
// Either the primary stream just cancelled and we have no fallback
// or we are the second stream to cancel.
// Signal the cancel.
if (mFallbackStream == null || mOnlyOneStreamRemains.getAndSet(true)) {
mBackendCallback.onCanceled(
CronetAdaptiveNetworkBidirectionalStream.this, info);
return;
}Erik OveliusSpurious diffs.
They indentation fixes.. I think this CL is small enough that it shouldn't matter that much. WDYT? (It was Gemini btw hehe)
if (mFallbackStream == null) {
return mPrimaryStream.isDone();
}
return mPrimaryStream.isDone() && mFallbackStream.isDone();Mohannad FarragI think the following would be easier to read:
```
bool fallbackIsDone = mFallbackStream == null ? true : mFallbackStream.isDone();
return mPrimaryStream.isDone() && fallbackIsDone;
```Or even:
```
return mPrimaryStream.isDone() && (mFallbackStream == null ? true : mFallbackStream.isDone());
```Or even:
```
return (mActiveStream.get() != null) ? mActiveStream.get().isDone() : (mPrimaryStream.isDone() && (mFallbackStream == null ? true : mFallbackStream.isDone()));
```
Now that you write it that way, I think there's a little bit more complication here. If the `mActiveStream` has successfully finished the request with a terminal callback of `onSucceeded` and the `mFallback` stream exists, then the result of `isDone()` should be true.
I always get confused about this thinking that the `fallback` stream would fire if the `primary` fails in a terminal callback, but that's not true.
Can't we just do `mActiveStream.get() != null ? mActiveStream.isDone() : false`? So if there's no active stream then that means either:
1) The request did not start yet.
2) The request started but has not reached `onStreamReady`If there's an active stream then it won't change in the future regardless of the outcome so just use `isDone()` on it.
edechamps@ first suggestion looks like a clear improvement 😊 I'd be careful trying to compress everything to one line though...
aymanm@: The third state missing is (let's pretend fallback doesn't exist at all) is:
3) stream started, never became active, but failed -> isDone() should be true then.
Thanks!
Related details
- Etienne Dechamps
- Mohannad Farrag
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohannad Farrag (Gerrit)
Mohannad Farrag voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mohannad Farrag
| Code-Review | +1 |
1 comment
mBackendCallback.onFailed(Is this the result of `git cl format?`
Related details
- Erik Ovelius
- Etienne Dechamps
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Erik Ovelius (Gerrit)
Erik Ovelius added 1 comment![]( "Open in Gerrit")
Is this the result of `git cl format?`
Now proper git cl format is done. It caught some more things. Updated description. Good call!
Related details
- Etienne Dechamps
- Mohannad Farrag
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohannad Farrag (Gerrit)
Mohannad Farrag voted Code-Review+1![]( "Open in Gerrit")
- Erik Ovelius
- Etienne Dechamps
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Dechamps (Gerrit)
Etienne Dechamps added 1 comment![]( "Open in Gerrit")
Erik OveliusSpurious diffs.
They indentation fixes.. I think this CL is small enough that it shouldn't matter that much. WDYT? (It was Gemini btw hehe)
How come this file was not formatted in the first place? `git cl upload` should not have allowed this. Can you fix the formatting across the whole file, send that as a CL, and then rebase this CL on top of that one? (For the test file too)
Related details
- Erik Ovelius
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Erik Ovelius (Gerrit)
Erik Ovelius added 1 comment![]( "Open in Gerrit")
Erik OveliusSpurious diffs.
Etienne DechampsThey indentation fixes.. I think this CL is small enough that it shouldn't matter that much. WDYT? (It was Gemini btw hehe)
How come this file was not formatted in the first place? `git cl upload` should not have allowed this. Can you fix the formatting across the whole file, send that as a CL, and then rebase this CL on top of that one? (For the test file too)
I don't know - I also assumed it was an upload check 😊 In Google3 this would have been auto formatted and blocked.
Do you want me to revert the formatting fixes from this change?
Given that this code isn't referenced anywhere yet - I'm hoping we can avoid that bureaucratic exercise and just submit this with the formatting fixes. WDYT?
Related details
- Etienne Dechamps
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Erik Ovelius (Gerrit)
I'd favor go/review-speed#why over nit whitespace/formatting changes... but if there is context I'm not understanding please let me know.. thanks!
Related details
- Etienne Dechamps
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Erik Ovelius (Gerrit)
Thank you for https://chromium-review.googlesource.com/c/chromium/src/+/7588433 - this would have taken me much longer 😊
I think we're synced up now, PTAL.
Related details
- Etienne Dechamps
- Stefano Duo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |