You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/14 14:29:57 UTC
[GitHub] [arrow] pitrou opened a new pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
pitrou opened a new pull request #10025:
URL: https://github.com/apache/arrow/pull/10025
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819656307
It's only for consistency, since `deque` is used for the threadpool executor.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819584593
True, it's just a potentially easy pitfall (though it comes with the territory I suppose).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace edited a comment on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
westonpace edited a comment on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819655814
Thanks @pitrou . A few thoughts (nothing worrisome):
Why `deque` instead of `queue`? I don't mind the change. I just want to make sure I'm not missing some piece of knowledge.
> Things may get tricky if cancellation or early exit is involved indeed...\
Cancellation should still flush the task queue. Early exit (via bad `Status`) should still flush the task queue. It is important we do because in both cases the remaining tasks might need to do some cleanup to handle the error. Exceptions/assertions might not but we're already in crash territory there.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819586365
Seems Crossbow built Arrow master, not this branch. I threw up https://github.com/lidavidm/crossbow/actions/runs/748753207
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819563110
I've checked locally that this fixes the TSan failures.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819611971
Github Actions: https://github.com/pitrou/arrow/actions?query=branch%3AARROW-12379-tsan++
Travis-CI: https://travis-ci.com/github/pitrou/arrow/builds/223062391
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819573595
Additionally, since we're handing around Executor* instead of shared_ptr<Executor> or something, there's still the possibility that an I/O thread tries to use a dangling reference, right? The main thread has to make sure to block and wait for all I/O threads it spawns under all conditions.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819564170
Submitted crossbow build: https://github.com/ursacomputing/crossbow/branches/all?query=build-226
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819610309
The crossbow job passes!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819748956
https://issues.apache.org/jira/browse/ARROW-12379
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm closed pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
lidavidm closed pull request #10025:
URL: https://github.com/apache/arrow/pull/10025
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819585201
Things may get tricky if cancellation or early exit is involved indeed...
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819562969
cc @westonpace
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819645197
Seemed the integration build timed out in Java but everything else passed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819655814
Thanks @pitrou . A few thoughts (nothing worrisome):
Why `deque` instead of `queue`? I don't mind the change. I just want to make sure I'm not missing some piece of knowledge.
> Things may get tricky if cancellation or early exit is involved indeed...
Cancellation should still flush the task queue. Early exit (via bad `Status`) should still flush the task queue. It is important we do because in both cases the remaining tasks might need to do some cleanup to handle the error. Exceptions/assertions might not but we're already in crash territory there.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819582413
Well, the idea is that the top-level task waits for all dependent tasks to finish, I think.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819569230
Yes.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10025: ARROW-12379: [C++] Fix ThreadSanitizer failure in SerialExecutor
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10025:
URL: https://github.com/apache/arrow/pull/10025#issuecomment-819570693
Hm, wait, I shouldn't unlock before notifying in MarkFinished().
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org