Deeply nested ServerTimestamps cause StackOverflowError #4702
Assignees
Labels
Comments
|
Hi @sk-takahiro-kurebayashi, thanks for reporting. Any chance you could share us a code snippet or minimal repro of your issue? It'll help us a lot. |
|
@argzdev I tried to reproduce the situation raising @Test
public void testDeeplyNestedServerTimestamps() {
Timestamp timestamp = Timestamp.now();
Value initialServerTimestamp = ServerTimestamps.valueOf(timestamp, null);
Map<String, Value> fields = new HashMap<String, Value>() {{
put("timestamp", ServerTimestamps.valueOf(timestamp, initialServerTimestamp));
}};
FieldPath path = FieldPath.fromSingleSegment("timestamp");
FieldMask mask = FieldMask.fromSet(new HashSet<FieldPath>() {{ add(path); }});
FieldTransform fieldTransform = new FieldTransform(path, ServerTimestampOperation.getInstance());
List<FieldTransform> fieldTransforms = new ArrayList<FieldTransform>() {{ add(fieldTransform); }};
AtomicReference<Throwable> error = new AtomicReference<>();
Thread thread = new Thread(Thread.currentThread().getThreadGroup(), () -> {
try {
for (int i = 0; i < 10000; ++i) {
writeMutation(new PatchMutation(DocumentKey.fromPathString("some/object/for/test"), ObjectValue.fromMap(fields), mask, Precondition.NONE, fieldTransforms));
}
} catch (Throwable e) {
error.set(e);
}
}, "test", 1024 * 1024);
try {
thread.start();
thread.join();
} catch (InterruptedException e) {
throw new AssertionError(e);
}
assertThat(error.get()).isNull();
}This unit test fails with below output. This stack-trace is shorten by too deep stack. If you might want to see more deeply, would you change stack size of Thread (currently set as 1024KB) like |
|
Thanks @sk-takahiro-kurebayashi . This is very helpful. I'll look into this issue |
|
Hi @sk-takahiro-kurebayashi , thank you for your thorough investigation. I've reviewed your suggested fix and test, and I think it's the right way forward. Would you like to make a pull-request and contribute to the project? :) Since you've done all the leg work, we'd love to have your name on it! If not, let me know and I'll do the pull-request. |
|
Thanks @sk-takahiro-kurebayashi , nothing else is needed by your side. I'll do a code review and let you know if there's any action needed by you. Thanks. |
|
Hi @ehsannas, thank you for your help! |
ehsannas
added a commit
that referenced
this issue
Mar 8, 2023
* Adds a new test of StackOverflowError caused by deeply nested ServerTimestamps See: #4702 * Keeps just only oldest previous value. Omit history of server-timestamps to fix StackOverflowError. See: #4702 * Fix formatting. * Add changelog. --------- Co-authored-by: Ehsan Nasiri <[email protected]>
|
Fixed by #4715 |
|
Reopening until the new PR lands (#4771) |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
[REQUIRED] Step 2: Describe your environment
[REQUIRED] Step 3: Describe the problem
Steps to reproduce:
@ServerTimestampannotation by “Set” operation with “merge” option.SQLiteDocumentOverlayCache#saveOverlayordecodeOverlaythrowsStackOverflowErrorwhile serializing/deserializing nested "ServerTimestamp" (MapValue) objects.Relevant Code:
IMHO, no need to keep nested previous value of "ServerTimestamp" object. And, just the oldest value is needed.
So, below patch resolves "deeply nested ServerTimestamp". But, saved overlay causes StackOverflow when it is parsed.
The text was updated successfully, but these errors were encountered: