Skip to content

fix: max message size not included in tornado settings.#1160

Open
ikwilnaarhuisman wants to merge 8 commits intoRobotWebTools:ros2from
eurogroep:fix/max_message_size-not-respected-in-tornado
Open

fix: max message size not included in tornado settings.#1160
ikwilnaarhuisman wants to merge 8 commits intoRobotWebTools:ros2from
eurogroep:fix/max_message_size-not-respected-in-tornado

Conversation

@ikwilnaarhuisman
Copy link
Copy Markdown

The max message size used to be implicitly used by the tornado settings, but this was broken in commit 949d7c3.

Because what used to happen is that the tornado.web.Application could implicitly find the attribute about the max_message_size, but because this was moved to a dictionary it can no longer. Causing Tornado to automatically disconnect when it receives a message exceeding 10 megabytes.

@YannickdeHoop

@bjsowa
Copy link
Copy Markdown
Member

bjsowa commented Feb 25, 2026

It would be helpful to add a test to rosbridge_server which tests this behavior so this does not happen in the future. Could you write one?

@ikwilnaarhuisman
Copy link
Copy Markdown
Author

I had a look at implementing a unit test where you could assert the max_message_size with the tornado max_message_size, but because tornado doesn't expose its parameters at runtime. I can't access it. I'm not sure how to unit test it without adding extra functionality.

@ikwilnaarhuisman
Copy link
Copy Markdown
Author

Please note that when tornado receives messages bigger than the max_message_size the connection is terminated. Making this is a very annoying issue.

https://github.com/tornadoweb/tornado/blob/3dfc965b2828fb2e48b700b1436dfbd16224a05d/tornado/websocket.py#L323

@YannickdeHoop
Copy link
Copy Markdown

@bjsowa Do you have any suggestions with regard to testing?

@bjsowa
Copy link
Copy Markdown
Member

bjsowa commented Mar 9, 2026

@bjsowa Do you have any suggestions with regard to testing?

If we want to define a certain behavior, IMO we should write an integration test which asserts this behavior, for example, by sending large messages to the server and testing how different values for max_message_size affect it.

@bjsowa bjsowa added this to the 3.2.0 milestone Mar 10, 2026
rick added 2 commits March 10, 2026 14:31
…s and allowed for unit tests to have an assertion error
@bjsowa bjsowa self-assigned this Mar 10, 2026
@bjsowa
Copy link
Copy Markdown
Member

bjsowa commented Mar 10, 2026

The test got stuck so I cancelled the workflow

@ikwilnaarhuisman
Copy link
Copy Markdown
Author

ikwilnaarhuisman commented Mar 10, 2026

They run locally (on jazzy) without a problem.

Other than mypy, whatever happens there.

@ikwilnaarhuisman
Copy link
Copy Markdown
Author

@bjsowa I removed some floating debug code. Be sure to quit the workflow if required

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants