Skip to content

Conversation

jinty
Copy link

@jinty jinty commented Jun 27, 2013

If you get an exception on the commit() call the
browser may see an OK message. This is because the
result is already passed to the WSGI server by the
time commit() is called. What actually happens
depends on the server.

It is a very bas idea to return a "Your changes
have been saved" message when, in fact, they have not.

This can happen if you have work happen on commit().
For example, SQLAlchemy will often only flush on commit
or a DEFERRED PostgreSQL constraint.

This is just a test as the solution is non-trivial.
Simply keeping the WSGI result in memory until the
commit() is called is not workable for large responses
or streaming (see 4b68c7d)

Neither does the WSGI spec provide any solace:
http://www.python.org/dev/peps/pep-3333/#error-handling

It is a very bas idea to return a "Your changes
have been saved" message when, in fact, they have
not…
@jinty
Copy link
Author

jinty commented Jun 27, 2013

https://github.com/Pylons/pyramid_tm does not have this problem AFAIKS, but then it also cannot handle large responses or streaming

@tseaver
Copy link
Member

tseaver commented Nov 27, 2013

Sorry for the delay in responding! I'm not sure what to do with the PR; we don't leave failing tests on master. Any new ideas about a solution?

@jinty
Copy link
Author

jinty commented Nov 27, 2013

at the moment, my strategy is to move to pyramid_tm where possible;)

But I think that what should to happen is that by default things should be safe, i.e. the commit must happen before the response is returned. This would be backwards incompatible and break streaming support. There could be a global or per-request switch to get streaming support back at the cost of absolutely correct transactions.

If breaking "streaming support by default" is acceptable, I can work on a patch.

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.

2 participants