Update WebSocket text handling to accumulate chunks before parsing #277
Conversation
|
Thanks @LuaKT! I think we should also add a test here |
| webSocket.request(1); | ||
|
|
||
| if (!last) { | ||
| return messageCompleteFuture; |
There was a problem hiding this comment.
I think we can just return null in onText if we are not sending a response. I know the original code returned CompletableFuture.completedStage(null), but futher research indicates it would be more straighforward to return null.
| } catch (Exception e) { | ||
| logger.error("Conductor Response error", e); | ||
| return CompletableFuture.completedStage(null); | ||
| return cf; |
There was a problem hiding this comment.
return null (see previous comment)
| } catch (Exception e) { | ||
| logger.error("Conductor JSON Parsing error", e); | ||
| return CompletableFuture.completedStage(null); | ||
| return cf; |
There was a problem hiding this comment.
return null (see previous comment)
|
FYI, #271 will require a pretty significant revamp of the conductor code. We test workflow import/export in Python with 100MB JSON files. We'll probably want to switch to incrementally parsing the JSON instead of collecting text chunks, combining into a 100MB string and then feeding that to the JSON parser. However, I'm not going to have time to work on #271 until the 2nd half of this week at the earliest. If this is blocking you @LuaKT, I'm OK merging this (with suggested changes + at least one test) in the meantime. |
|
Hi @devhawk, |
2 participants
WebSocket messages may arrive in multiple chunks. This change accumulates all chunks before processing to ensure complete messages are parsed.
Fixes #276