You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "BryanCutler (via GitHub)" <gi...@apache.org> on 2023/10/06 21:52:12 UTC
[PR] GH-38096: [Java] FlightStream with metadata can cause error when closing [arrow]
BryanCutler opened a new pull request, #38110:
URL: https://github.com/apache/arrow/pull/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()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38096: [Java] FlightStream with metadata can cause error when closing [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #38110:
URL: https://github.com/apache/arrow/pull/38110#discussion_r1355001623
##########
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightClient.java:
##########
@@ -412,6 +412,16 @@ public ClientStreamListener getWriter() {
return writer;
}
+ /**
+ * Make sure stream is drained. You must call this to be notified of any errors that may have
+ * happened after the exchange is complete. This should be called after `getWriter().completed()`
+ * and instead of `getWriter().getResult()`.
+ */
+ public void getResult() {
+ // After exchange is complete, make sure stream is drained to propagate errors through reader
+ reader.next();
Review Comment:
Possibly loop while reader.next()?
##########
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;
Review Comment:
Should this go in the finally instead? I suppose if this throws the application is unlikely to close again.
##########
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightClient.java:
##########
@@ -412,6 +412,16 @@ public ClientStreamListener getWriter() {
return writer;
}
+ /**
+ * Make sure stream is drained. You must call this to be notified of any errors that may have
+ * happened after the exchange is complete. This should be called after `getWriter().completed()`
+ * and instead of `getWriter().getResult()`.
+ */
+ public void getResult() {
+ // 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 {
Review Comment:
Should we drain the stream in close as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38096: [Java] FlightStream with metadata can cause error when closing [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #38110:
URL: https://github.com/apache/arrow/pull/38110
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38096: [Java] FlightStream with metadata can cause error when closing [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #38110:
URL: https://github.com/apache/arrow/pull/38110#discussion_r1355787513
##########
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightClient.java:
##########
@@ -412,6 +412,16 @@ public ClientStreamListener getWriter() {
return writer;
}
+ /**
+ * Make sure stream is drained. You must call this to be notified of any errors that may have
+ * happened after the exchange is complete. This should be called after `getWriter().completed()`
+ * and instead of `getWriter().getResult()`.
+ */
+ public void getResult() {
+ // 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 {
Review Comment:
Sounds good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38096: [Java] FlightStream with metadata can cause error when closing [arrow]
Posted by "BryanCutler (via GitHub)" <gi...@apache.org>.
BryanCutler commented on PR #38110:
URL: https://github.com/apache/arrow/pull/38110#issuecomment-1758547383
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38096: [Java] FlightStream with metadata can cause error when closing [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38110:
URL: https://github.com/apache/arrow/pull/38110#issuecomment-1764246975
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 71a4ef4b1014c22e8bc7627bb9c66efa20e1b453.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/17736669918) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38096: [Java] FlightStream with metadata can cause error when closing [arrow]
Posted by "BryanCutler (via GitHub)" <gi...@apache.org>.
BryanCutler commented on PR #38110:
URL: https://github.com/apache/arrow/pull/38110#issuecomment-1762375781
Thanks @lidavidm !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38096: [Java] FlightStream with metadata can cause error when closing [arrow]
Posted by "BryanCutler (via GitHub)" <gi...@apache.org>.
BryanCutler commented on code in PR #38110:
URL: https://github.com/apache/arrow/pull/38110#discussion_r1355775352
##########
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;
Review Comment:
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 I don't think the application could recover.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38096: [Java] FlightStream with metadata can cause error when closing [arrow]
Posted by "BryanCutler (via GitHub)" <gi...@apache.org>.
BryanCutler commented on code in PR #38110:
URL: https://github.com/apache/arrow/pull/38110#discussion_r1355775352
##########
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;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38096: [Java] FlightStream with metadata can cause error when closing [arrow]
Posted by "BryanCutler (via GitHub)" <gi...@apache.org>.
BryanCutler commented on PR #38110:
URL: https://github.com/apache/arrow/pull/38110#issuecomment-1751445924
@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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38096: [Java] FlightStream with metadata can cause error when closing [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #38110:
URL: https://github.com/apache/arrow/pull/38110#issuecomment-1757691350
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38096: [Java] FlightStream with metadata can cause error when closing [arrow]
Posted by "BryanCutler (via GitHub)" <gi...@apache.org>.
BryanCutler commented on code in PR #38110:
URL: https://github.com/apache/arrow/pull/38110#discussion_r1355784469
##########
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightClient.java:
##########
@@ -412,6 +412,16 @@ public ClientStreamListener getWriter() {
return writer;
}
+ /**
+ * Make sure stream is drained. You must call this to be notified of any errors that may have
+ * happened after the exchange is complete. This should be called after `getWriter().completed()`
+ * and instead of `getWriter().getResult()`.
+ */
+ public void getResult() {
+ // 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 {
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org