GH-38096: [Java] FlightStream with metadata can cause error when closing #38110

BryanCutler · 2023-10-06T21:52:10Z

Rationale for this change

The Java FlightStream can raise an error if metadata is transferred and ends up being closed twice.

java.lang.IllegalStateException: RefCnt has gone negative
	at org.apache.arrow.util.Preconditions.checkState(Preconditions.java:458)
	at org.apache.arrow.memory.BufferLedger.release(BufferLedger.java:130)
	at org.apache.arrow.memory.BufferLedger.release(BufferLedger.java:104)
	at org.apache.arrow.memory.ArrowBuf.close(ArrowBuf.java:1044)
	at org.apache.arrow.util.AutoCloseables.close(AutoCloseables.java:97)
	at org.apache.arrow.flight.FlightStream.close(FlightStream.java:208)

What changes are included in this PR?

When FlightStream is closed, remove any reference of previous metadata to prevent reference count going negative if closed again. Also added ExchangeReaderWriter.getResult() for convenience and clear up ambiguity on error handling.

Are these changes tested?

Unit tests added for closing with metadata and

Are there any user-facing changes?

Added ExchangeReaderWriter.getResult()

Closes: [Java] Flight stream with metadata gives an error if closed twice #38096

BryanCutler · 2023-10-06T21:59:35Z

@lidavidm I was playing around with the DoExchange call and noticed a couple of issues. At first I was a little confused as to why getWriter().getResult() wasn't being called until I realized that errors were coming through the reader. If getWriter().getResult() is called before getReader().next() is called a final time, then it seems to hang.

So I noticed that you really need to call getReader().next() a final time, so it will return false or raise any errors that happened after exchanging all the batches. To hopefully make this more clear to the user I added ExchangeReaderWriter.getResult() so it will be obvious to call this instead. I'm not 100% sure that it's a good idea, so let me know what you think. Thanks!

lidavidm

Thanks, just some nits

lidavidm · 2023-10-11T13:19:54Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightClient.java

+     */
+    public void getResult() {
+      // After exchange is complete, make sure stream is drained to propagate errors through reader
+      reader.next();


Possibly loop while reader.next()?

lidavidm · 2023-10-11T13:20:16Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightStream.java

@@ -207,6 +207,8 @@ public void close() throws Exception {
        } else {
          AutoCloseables.close(closeables);
        }
+        // Remove any metadata after closing to prevent negative refcnt
+        applicationMetadata = null;


Should this go in the finally instead? I suppose if this throws the application is unlikely to close again.

I put it here because if something throws and error before this, it's likely the buffer was not closed then. So at least this way, the buffer could be examined, but either way there is probably some resources leaked.

lidavidm · 2023-10-11T13:20:47Z

java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightClient.java

+      // After exchange is complete, make sure stream is drained to propagate errors through reader
+      reader.next();
+    }
+
    /** Shut down the streams in this call. */
    @Override
    public void close() throws Exception {


Should we drain the stream in close as well?

It is drained already in the call to reader.close(). From what I can tell, it will suppress any exception though. So probably better off draining with the call to getResult() before closing to handle any errors, wdyt?

Sounds good.

lidavidm · 2023-10-11T13:22:07Z

At first I was a little confused as to why getWriter().getResult() wasn't being called until I realized that errors were coming through the reader.

We try to expose 'independent' readers/writers but in actuality in gRPC they are tied + the reader is the only source of errors (which in some sense makes sense, I suppose) so this is perhaps more of a mistake in our API design

BryanCutler · 2023-10-11T21:10:21Z

We try to expose 'independent' readers/writers but in actuality in gRPC they are tied + the reader is the only source of errors (which in some sense makes sense, I suppose) so this is perhaps more of a mistake in our API design

Makes sense, thanks for the explanation

…n closing (apache#38110) ### Rationale for this change The Java FlightStream can raise an error if metadata is transferred and ends up being closed twice. ``` java.lang.IllegalStateException: RefCnt has gone negative at org.apache.arrow.util.Preconditions.checkState(Preconditions.java:458) at org.apache.arrow.memory.BufferLedger.release(BufferLedger.java:130) at org.apache.arrow.memory.BufferLedger.release(BufferLedger.java:104) at org.apache.arrow.memory.ArrowBuf.close(ArrowBuf.java:1044) at org.apache.arrow.util.AutoCloseables.close(AutoCloseables.java:97) at org.apache.arrow.flight.FlightStream.close(FlightStream.java:208) ``` ### What changes are included in this PR? When FlightStream is closed, remove any reference of previous metadata to prevent reference count going negative if closed again. Also added `ExchangeReaderWriter.getResult()` for convenience and clear up ambiguity on error handling. ### Are these changes tested? Unit tests added for closing with metadata and ### Are there any user-facing changes? Added `ExchangeReaderWriter.getResult()` * Closes: apache#38096 Authored-by: Bryan Cutler <[email protected]> Signed-off-by: David Li <[email protected]>

BryanCutler · 2023-10-13T23:32:44Z

Thanks @lidavidm !

conbench-apache-arrow · 2023-10-16T11:14:35Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 71a4ef4.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…n closing (apache#38110) ### Rationale for this change The Java FlightStream can raise an error if metadata is transferred and ends up being closed twice. ``` java.lang.IllegalStateException: RefCnt has gone negative at org.apache.arrow.util.Preconditions.checkState(Preconditions.java:458) at org.apache.arrow.memory.BufferLedger.release(BufferLedger.java:130) at org.apache.arrow.memory.BufferLedger.release(BufferLedger.java:104) at org.apache.arrow.memory.ArrowBuf.close(ArrowBuf.java:1044) at org.apache.arrow.util.AutoCloseables.close(AutoCloseables.java:97) at org.apache.arrow.flight.FlightStream.close(FlightStream.java:208) ``` ### What changes are included in this PR? When FlightStream is closed, remove any reference of previous metadata to prevent reference count going negative if closed again. Also added `ExchangeReaderWriter.getResult()` for convenience and clear up ambiguity on error handling. ### Are these changes tested? Unit tests added for closing with metadata and ### Are there any user-facing changes? Added `ExchangeReaderWriter.getResult()` * Closes: apache#38096 Authored-by: Bryan Cutler <[email protected]> Signed-off-by: David Li <[email protected]>

BryanCutler requested a review from lidavidm as a code owner October 6, 2023 21:52

github-actions bot added Component: Java awaiting review Awaiting review labels Oct 6, 2023

lidavidm approved these changes Oct 11, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Oct 11, 2023

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Oct 11, 2023

BryanCutler added 5 commits October 11, 2023 14:16

Reset metadata to null in FlightStream.close()

28d1f90

Add test for double-close and handling error in DoExchange

da5ccef

Add ExchangeReaderWriter.getResult()

e4100f3

Fixed comment

34360b8

Use loop to drain exchange reader

7740c76

BryanCutler force-pushed the java-flight-stream-meta-close-38096 branch from 9849256 to 7740c76 Compare October 11, 2023 21:16

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 11, 2023

lidavidm approved these changes Oct 12, 2023

View reviewed changes

lidavidm merged commit 71a4ef4 into apache:main Oct 12, 2023

lidavidm removed the awaiting change review Awaiting change review label Oct 12, 2023

github-actions bot added the awaiting merge Awaiting merge label Oct 12, 2023

BryanCutler deleted the java-flight-stream-meta-close-38096 branch October 13, 2023 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38096: [Java] FlightStream with metadata can cause error when closing #38110

GH-38096: [Java] FlightStream with metadata can cause error when closing #38110

BryanCutler commented Oct 6, 2023 •

edited by github-actions bot

Loading

BryanCutler commented Oct 6, 2023

lidavidm left a comment

lidavidm Oct 11, 2023

lidavidm Oct 11, 2023

BryanCutler Oct 11, 2023 •

edited

Loading

lidavidm Oct 11, 2023

BryanCutler Oct 11, 2023

lidavidm Oct 11, 2023

lidavidm commented Oct 11, 2023

BryanCutler commented Oct 11, 2023

BryanCutler commented Oct 13, 2023

conbench-apache-arrow bot commented Oct 16, 2023

GH-38096: [Java] FlightStream with metadata can cause error when closing #38110

GH-38096: [Java] FlightStream with metadata can cause error when closing #38110

Conversation

BryanCutler commented Oct 6, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

BryanCutler commented Oct 6, 2023

lidavidm left a comment

Choose a reason for hiding this comment

lidavidm Oct 11, 2023

Choose a reason for hiding this comment

lidavidm Oct 11, 2023

Choose a reason for hiding this comment

BryanCutler Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

lidavidm Oct 11, 2023

Choose a reason for hiding this comment

BryanCutler Oct 11, 2023

Choose a reason for hiding this comment

lidavidm Oct 11, 2023

Choose a reason for hiding this comment

lidavidm commented Oct 11, 2023

BryanCutler commented Oct 11, 2023

BryanCutler commented Oct 13, 2023

conbench-apache-arrow bot commented Oct 16, 2023

BryanCutler commented Oct 6, 2023 •

edited by github-actions bot

Loading

BryanCutler Oct 11, 2023 •

edited

Loading