-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core dump crash #29353
Comments
@nodejs/http2 |
@jakeleventhal Thanks for the report! Any chance you could share reproduction code? It may seem like “nothing complicated” to you or other people familiar with the firestore module, but it would be very helpful to have something a bit more concrete. |
Here is the function that is being ran (I changed some variable names and removed some intermediate steps that are just manipulating variables - nothing that has to do with promises or networking). From the looks of it, there is nothing inconspicuous going on here export const saveReportData = async (cateogories, reportData) => {
// Create the map of updates
const allUpdates = {};
cateogories.forEach((category) => {
allUpdates[category] = { items: {} };
});
await Promise.all(
reportData
.filter(
({ channel }) => cateogories.includes(channel)
)
.map(
async ({
id,
email,
}) => {
// Get the existing item
const existingItem = await firestore.doc(`Items/${id}`).get();
// If the item is already saved in the database and hasn't been processed yet
if (existingItem.exists && !existingItem.data().Processed) {
const itemUpdatePayload = {
Processed: true,
User: email,
Status: 'Complete'
};
// Update the item in the database
await firestore.doc(`Items/${id}`).update(itemUpdatePayload);
}
}
)
);
}; |
Unfortunately, I can’t reproduce an error with that code (ran it with dummy input We could just remove that check, but this does point to a bigger bug in the HTTP/2 implementation of some kind, and so it would be really good to figure out what that is. |
Like i said in the other issue, I can’t get it to reliably reproduce. In fact it’s the minority of cases. And it appears to always happen after a deadline exceeded error. on a side note, how are these deadline exceeded errors avoided anyway |
I’m afraid that’s a question for the GCP folks. |
To clarify that a bit more, the deadline is a gRPC specific concept - you can see the stack trace contains |
Okay, well if it isn’t going to break anything, can the check be removed? At least until the source of the problem is found |
Don’t start reading more input data if we’re still busy writing output. This was overlooked in 8a4a193. Fixes: nodejs#29353 Fixes: nodejs#29393
#29399 should fix this. |
Hopefully. How long does it take for node to release updates though...? |
@jakeleventhal For latest probably 1-2 weeks. |
Could this also be affecting older versions? Seeing something similar on v10.16. If so, how long would it take to get a fix released to 10.16? |
@addaleax is there any validation plan for this? I am unable to reliably produce the error |
Don’t start reading more input data if we’re still busy writing output. This was overlooked in 8a4a193. Fixes: nodejs#29353 Fixes: nodejs#29393 PR-URL: nodejs#29399 Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Don’t start reading more input data if we’re still busy writing output. This was overlooked in 8a4a193. Fixes: nodejs#29353 Fixes: nodejs#29393 PR-URL: nodejs#29399 Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Don’t start reading more input data if we’re still busy writing output. This was overlooked in 8a4a193. Fixes: #29353 Fixes: #29393 PR-URL: #29399 Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Don’t start reading more input data if we’re still busy writing output. This was overlooked in 8a4a193. Fixes: #29353 Fixes: #29393 PR-URL: #29399 Backport-PR-URL: #29618 Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Don’t start reading more input data if we’re still busy writing output. This was overlooked in 8a4a193. Fixes: #29353 Fixes: #29393 PR-URL: #29399 Backport-PR-URL: #29619 Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
node:latest
docker imagePlease see the issue here googleapis/nodejs-firestore#739
This describes details of how I was able to reproduce this problem using the
@google-cloud/firestore
module.Regardless of the module, node should not be crashing like this.
The text was updated successfully, but these errors were encountered: