Skip to content
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

Allow user to specify request buffer size. #4

Closed
wants to merge 1 commit into from

Conversation

cthulahoops
Copy link
Contributor

The next limit we ran into was and 1kb buffer for reading the request. Not knowing how much memory different devices have, it seemed best to expose the buffer size rather than just increase it.

Co-authored-by: Shae Erisson shae@scannedinavian.com

Co-authored-by: Shae Erisson <shae@scannedinavian.com>
@dhalbert
Copy link
Contributor

I wrote this library in a hurry for a specific use case, hence the 0.1.x version. It could do partial `recv's and build up the request instead of having to specify the biggest possible buffer, so that the buffer size becomes irrelevant.

There are some other libraries that you might look at as well. I am not sure about what your requirements are. Are you serving files or mainly doing routing?
https://github.com/Neradoc/circuitpython-native-wsgiserver/
https://github.com/adafruit/Adafruit_CircuitPython_WSGI

@cthulahoops
Copy link
Contributor Author

We thought about doing the partial receives approach, but ended up going with the quick fix here. We could probably look a implementing that if you like.

We have a bunch of neopixels, and we're sending animation frames as http requests - the encoding is pretty naive so one of the requests ended up at about 1.5 kb. There's another pending change to add support for query parameters that's not quite ready, and then I think this library does everything we need. (So far, at least!)

We haven't looked at the wsgi libraries. Thanks for linking them, we'll take look!

@dhalbert
Copy link
Contributor

We haven't looked at the wsgi libraries. Thanks for linking them, we'll take look!

Given that you are not trying to fetch large files (which was the main purpose of this quickly-made library), yes, the wsgi libraries are probably in better shape for what you want to do. We have talked about replacing this library with an adapted version of @Neradoc's https://github.com/Neradoc/circuitpython-native-wsgiserver/ and adding built-in file fetching to that library.

@FoamyGuy
Copy link
Contributor

@dhalbert understanding that WSGI library may be a better fit for this use case. Do you have thoughts on the specific change here? Any potential problems with allowing the argument to be passed here to change the buffer size? It seems okay to me, but I'm not sure I'd understand all of the ramifications.

@adafruit adafruit deleted a comment from FoamyGuy May 23, 2022
@dhalbert
Copy link
Contributor

[Deleted dupe comment.]

@FoamyGuy Adding the size argument creates a new API, which I'm not sure we want to support in the long run. The best fix is to make the size specification unnecessary by doing partial receives and concatenating the pieces as necessary

I only provided this library so we could do off-line-Wordle-like stuff quickly. We could include this PR, but I would like the whole thing to be re-evaluated and possibly replaced by a better base library.

@cthulahoops
Copy link
Contributor Author

I kind of agree that the expanded API here isn't worth supporting. We've worked around this with a local change, and can evaluate alternative libraries. Happy for this to be closed.

@dhalbert
Copy link
Contributor

Thanks, I will close it then, and open an issue for the general problem.

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.

None yet

3 participants